504977 - all copies of shexp.c are vulnerable

Status: VERIFIED DUPLICATE of bug 332173

(Firefox Build System :: General, defect)

Description

[Nelson Bolyard (seldom reads bugmail)]

** Embargoed until 2009-07-29 1800 UTC **

To resolve bug 498500, copies of shexp.c were copied/committed to 
modules/libjar/nsWildCard.cpp  and
toolkit/components/filepicker/src/nsWildCard.cpp 

If the pattern passed to these functions can come from an external source
(e.g., from a web page or other downloaded content), they are vulnerable 
to code execution attacks.  See bug 504456 for details.  

It would be good if we could find a single fix for bug 504456 and this bug.
that is, it would be good to find a single patch for all copies.

Assigning to Robert, who committed those copies.  Robert, I will let you determine which, if any, of these copies are actually vulnerable.

Comment 1

[Daniel Veditz [:dveditz]]

Thanks for filing this after our IM chat, Nelson. The embargo date assumes 1) the original reporter will mention this at BlackHat on that date and 2) has noticed that the reported problem extends to libjar and the filepicker.

I'm pretty sure bug 498500 has nothing (or little) to do with this problem. These files are used on the 1.8 branch, for instance:
http://mxr.mozilla.org/mozilla1.8/find?string=nsWildCard
(the libjar copy goes back to 1999)

The xpfe filepicker moved to toolkit for 1.9.1
http://mxr.mozilla.org/mozilla1.9.1/find?string=nsWildCard

and consolidated into xpcom/io on trunk
http://mxr.mozilla.org/mozilla-central/find?string=nsWildCard

Comment 2

[Daniel Veditz [:dveditz]]

We look OK on the libjar front, all uses of nsIZipReader.findEntries are specific files or simple baked-in "*" wildcard patterns (e.g. looking for files of a specific extension). Checking a large sampling of Addons most uses were similar, although a few had a pattern variable and we'd have to review them to make sure the pattern didn't come from user input.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

NSS's copy goes back to 1994. :)

I got the reference to bug 498500 from these pages:
http://hg.mozilla.org/releases/mozilla-1.9.1/annotate/1e7c406956d3/toolkit/components/filepicker/src/nsWildCard.cpp
http://hg.mozilla.org/releases/mozilla-1.9.1/annotate/1e7c406956d3/modules/libjar/nsWildCard.cpp
Maybe those pages are lying to me about the bug that is relevant to their checkins (wouldn't surprise me).

I wrote a patch for this shexp code and attached it to bug 504456. 
I think it plugs the vulnerability, but I'm not happy with it.  
That shexp code is a real piece of work.  It doesn't handle nesting or 
escaping properly.  Fixing it the right way would be a rewrite.  NSS doesn't
need it badly enough to warrant that, but maybe these other uses do.  
Maybe we should just toss this code out, and try to find some other open 
source implementation of file name "globbing", and use it instead.

Comment 4

[Robert Kaiser]

(In reply to comment #3)
> NSS's copy goes back to 1994. :)
> 
> I got the reference to bug 498500 from these pages:
> http://hg.mozilla.org/releases/mozilla-1.9.1/annotate/1e7c406956d3/toolkit/components/filepicker/src/nsWildCard.cpp
> http://hg.mozilla.org/releases/mozilla-1.9.1/annotate/1e7c406956d3/modules/libjar/nsWildCard.cpp
> Maybe those pages are lying to me about the bug that is relevant to their
> checkins (wouldn't surprise me).

Ugh, you misinterpreted the hgweb output completely, it just happens to show the latest commit that was made to the repository on top of the page, but that does have no connection if the shown file itself was even touched by this commit.
Click on the actual commit in left left column to see what changeset the affected line(s) was last changed in - in this case, the initial import of Mozilla source into hg, which has nothing to do with me ;-)

Comment 5

[Benjamin Smedberg]

This is bug 332173, I think.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Thanks, Benjamin.  I concur.

Robert, Thanks for the explanation.  Sorry for my confusion.  I'm not (yet?) 
an Hg user.  I find this idea that every page for every source file cites 
the most recent commit, whether it had ANYTHING to do with this file or not, 
to be unintuitive and not very useful.  Is there ANY way to get a page that
lists just the changesets that actually affected the file being viewed?

Comment 7

[Robert Kaiser]

Nelson, the "revisions" link, e.g. http://hg.mozilla.org/releases/mozilla-1.9.1/log/1e7c406956d3/modules/libjar/nsWildCard.cpp gives you all changesets that affected the file at all, in this case only the initial import into hg. In the annotate view you linked, the column of links in the left column (all "hg@1" in those links, i.e. done by the user "hg" in the first repository-wide revision) tells you which user/revision changed that specific line most recently. The revision and comment at the top tell you what revision of the repository you are looking at in general. The important concept here is that revisions/changesets are always for the whole repository and not per file as in CVS. With the revisions links you can look at revisions/changesets that affected a files though and with annotate at the last revision/changeset that affected a certain code line.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Thanks, Robert.
>  revisions/changesets are always for the whole repository and not
> per file as in CVS.
Yeah, that is precisely why Mozilla's Hg repositories are "downstream" 
repositories for NSS.  

BTW, I will shortly have a new patch for the old shexp.c code.  It's a 
pretty big change, but it's much better than what we have now, and must 
better than my previous patches to it.

Comment 9

[Samuel Sidler (old account; do not CC)]

Fixed by bug 332173.

Comment 10

[juan becerra [:juanb]]

Nelson, could you help us verify this for 3.5.2?

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

I hereby verify that this is a duplicate of bug 332173.