[Edbrowse-dev] wordexp

[Edbrowse-dev] wordexp

[Chris Brannon]

I recently learned that the wordexp function may have security issues on
some operating systems.
http://www.openwall.com/lists/oss-security/2015/02/11/3

So my question is this.  Are we passing any untrusted input to wordexp?
Or does it just expand filenames obtained from the user at the keyboard?

-- Chris

[Edbrowse-dev] wordexp

[Karl Dahlke]

This is called to expand variables in file names that the user types in.
I really don't think it should be a problem.

Karl Dahlke

Re: [Edbrowse-dev] wordexp

[Adam Thompson]

[-- Attachment #1: Type: text/plain, Size: 1264 bytes --]

On Tue, Dec 30, 2014 at 06:10:49AM -0500, Karl Dahlke wrote:
> After considerable waffling, I decided to give wordexp a try
> for variable expansion and globbing on file names.
> Odds are, we won't notice any difference.
> But at least it is somewhat standard now, and I got rid of a bunch of
> #ifdef Windows dos unix code.
> Some of the old code is still in there under #if 0,
> and I'll get rid of that when I have confidence in the new system.

Ok, everything appears to work ok, though I pushed a change to allow expansion of env variables etc for non-existant file names when writing or setting the file name.
For example:
export tmpdir=/tmp
then:
edbrowse
f $tmpdir/test.txt
w $tmpdir/test_2.txt

Now allows you to set the file name to /tmp/test.txt and write to /tmp/test_2.txt
rather than erroring as it did previously.
Note that, if the file doesn't exist, b,
r and e commands still error as expected.

> Speaking of old stuff, we'll have to decide when to delete
> jsdom.cpp jsloc.cpp js.h.
> That is, when are we committed to the new system?
> Of course, thanks to git, you never really delete stuff,
> you can always get it back if you want it.

Thus I'd say delete it now, since it appears to work just as well as the
previous system.

Cheers,
Adam.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[Edbrowse-dev] wordexp

[Karl Dahlke]

After considerable waffling, I decided to give wordexp a try
for variable expansion and globbing on file names.
Odds are, we won't notice any difference.
But at least it is somewhat standard now, and I got rid of a bunch of
#ifdef Windows dos unix code.
Some of the old code is still in there under #if 0,
and I'll get rid of that when I have confidence in the new system.

Speaking of old stuff, we'll have to decide when to delete
jsdom.cpp jsloc.cpp js.h.
That is, when are we committed to the new system?
Of course, thanks to git, you never really delete stuff,
you can always get it back if you want it.

Karl Dahlke

Re: [Edbrowse-dev] wordexp

[Adam Thompson]

[-- Attachment #1: Type: text/plain, Size: 2038 bytes --]

On Sun, Dec 28, 2014 at 04:45:34PM -0500, Karl Dahlke wrote:
> After some playing around with wordexp, I'm inclined to leave things alone.
> wordexp just does too much.
It certainly does a lot, but the existing code also does a fair amount,
and will still break on file names if they're not correctly escaped (which your
examples aren't). Also, I had to read the code to tell me how to
escape the meta characters (though I admit I may've just missed that one in the user's guide).
I'm certainly thinking that glob would be a worth while change to make here,
possibly keeping the hand-rolled env variable expansion?
On the subject of that, what happens if I have the following:
testfile=mytest
and the file:
mytest_22.txt
and try:
r $testfile_22.txt
This doesn't work as testfile_22 is not a variable,
and there appears to be no current way to stop expansion (note that we can't
ignore env vars with _ in them as they're legal in posix shell).
Basically what I'm getting at here is that at the moment the code is buggy and
at least if we use wordexp we can say that "your file names must be valid in a
posix shell" or words to that effect and then it becomes fairly clear what's
allowed (simply \ escape anything you're not sure about).
Thus the above example becomes:
r ${testfile}_22.txt
and a file name like:
it'd be unfortunate to get this.txt
becomes:
it\'d\ be\ unfortunate\ to\ get\ this.txt
or:
"it'd be unfortunate to get this.txt"

At the very least we should bring our variable expansion and escape character in line with something vaguely standard (i.e. optional braces for ambiguous cases in env variables and \ rather than `) imho. We should also probably use glob for... well... globbing I think.
I'm also not confident there aren't more bugs there if someone pokes it hard 
enough.
Having said all that, it'd probably be a nice idea to have a toggle command to
switch off file name expansion for when you really don't need it and have some
horrible file name to edit.

Cheers,
Adam.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[Edbrowse-dev] wordexp

[Karl Dahlke]

After some playing around with wordexp, I'm inclined to leave things alone.
wordexp just does too much.
Unbalanced parens or brackets or quotes or apostrophes
will produce a syntax error.
Here's a filename with one apostrophe in it.

#include <stdio.h>
#include <wordexp.h>
main()
{
wordexp_t w;
int rc;
rc = wordexp("isn't", &w, 0);
if(rc == WRDE_SYNTAX)
puts("syntax error");
}

And even if the quotes are balanced, they will be taken away from you.

#include <stdio.h>
#include <wordexp.h>
main()
{
wordexp_t w;
int rc;
rc = wordexp("is'n't", &w, 0);
puts(w.we_wordv[0]);
}

So many side effects that I'm sure people will be confused.
I already wrote the code and it's been working for years,
guess I'll leave it alone.

Karl Dahlke