631307 - (CVE-2011-0063) Possible to bypass fix for CVE-2011-0049 (majordomo2 directory traversal in 'help' command)

Status: VERIFIED FIXED

(Security Assurance :: General, task)

Description

[Reed Loden [:reed]]

Nikolas Sotiriu <[e-mail]> reported the following issue to security@ concerning a way to bypass the majordomo2 fix in bug 628064:

=================================================================
So the bug is that the majordomo2 path for the bug (628064) is absolut
terrible and not working.

See attachment 506481 [details] [diff] [review].

Check the regex ($file =~ s!/?\.\./?!!g;) do you see it :)

It deletes ../ but what happens if i ./.../ ?

./.../ becomes ../

http://bugzilla.org/cgi-bin/mj_wwwusr?passw=&list=GLOBAL&user=&func=help&extra=./..././..././..././..././..././..././..././.../etc/passwd

Maybe a regex like this is better:

$file =~ s/\.\.//g;
=================================================================

Comment 1

[Jason Tibbitts]

"absolut terrible and not working".  Wow.  I'm in here helping folks fix code I haven't worked on in years, and honestly think I deserve something better than abuse.

Now, great, that's clever and I never would have found it.  Sure wish someone could just communicate with me directly instead of flaming me in bugs I'm lucky to actually see.  I've committed a fix.

Comment 2

[Dave Miller [:justdave]]

He mailed it to our security@, probably didn't know you were on the bug (didn't post it there anyway).  Doesn't excuse the "verbal abuse" anyway.  I still appreciate you taking the time to help with this. :)

Comment 3

[Dave Miller [:justdave]]

Upstream patch pulled and applied.

For the record:

Index: lib/Majordomo.pm
===================================================================
RCS file: /home/cvs/majordomo/majordomo/lib/Majordomo.pm,v
retrieving revision 1.445
retrieving revision 1.446
diff -u -r1.445 -r1.446
--- lib/Majordomo.pm    24 Jan 2011 20:33:54 -0000      1.445
+++ lib/Majordomo.pm    3 Feb 2011 19:42:07 -0000       1.446
@@ -3521,7 +3521,7 @@
   my $lang = $args{lang};
 
   # $file might have bad stuff in it
-  $file =~ s!/?\.\./?!!g;
+  $file =~ s!\.\.!!g;
 
   # Account for list:sublist
   if ($list =~ /^([^:\s]+):/) {

Comment 4

[Dave Miller [:justdave]]

er, sorry, that diff is backwards.  swap the + and - as you read it :)

Comment 5

[Dave Miller [:justdave]]

actually, on further poking, I think that's actually the correct order.

Comment 6

[Dave Miller [:justdave]]

Yeah, the currently live code has:
  $file =~ s!\.\.!!g;

Comment 9

[Dave Miller [:justdave]]

I'm still not 100% sure on that regexp.  I know I've seen this done in other apps and I'm pretty sure the regexp was different.  But I can't put my finger on it, and I haven't managed to break this one yet.

Comment 10

[Jason Tibbitts]

Well, there are other possibilities.  Getting too clever by half in trying to strip out dots next to slashes obviously wasn't the right thing to do.  If you come up with something better I'm happy to use it.

http://stackoverflow.com/questions/1734145/is-this-specific-path-concatenation-in-perl-code-exploitable seems to be somewhat on point.  I could also detect any double-dot anywhere and instead of trying to manipulate the string just replace thw whole thing with the error document.

Anyway, always open to suggestions.  I make no claims at all to being any kind of security professional.

Comment 11

[Dave Miller [:justdave]]

No problem, thanks for all the help so far, you've been super responsive for someone that's been mostly away from the project for a few years. :)

Guess I'll own this for now, it's paging for being unassigned for too long at critical severity. :)

Comment 12

[lofi]

(In reply to comment #1)
> "absolut terrible and not working".  Wow.  I'm in here helping folks fix code I
> haven't worked on in years, and honestly think I deserve something better than
> abuse.
> 
> Now, great, that's clever and I never would have found it.  Sure wish someone
> could just communicate with me directly instead of flaming me in bugs I'm lucky
> to actually see.  I've committed a fix.


Sorry for that. i didn't know that this would make you so angry. It was not my intention to criticize your help to fix the problem.

The point was, that i'm not a perl programmer (just used it for some
exploits) but i directy saw that this patch didn't work.

So sorry again for the "verbal abuse"

BTW: Nice that the email was posted here, but the last passage is missing

> But i can't give any garanty that this will realy work, because i 
> don't have a majordomo installation here.

It looks like a new snapshot is out, which has the fixed regex http://ftp.mj2.org/pub/mj2/snapshots/2011-02/majordomo-20110204.tar.gz

What about the public disclosure?

Advisory draft: http://sotiriu.de/adv/NSOADV-2011-1268349720915729430439.txt

Comment 13

[Reed Loden [:reed]]

This is CVE-2011-0063.

Comment 14

[lofi]

Can this bug be made public, to inform the majordomo users to patch there systems?

Comment 15

[chris hofmann]

justdave and cshields,  can you get someone to mark this fixed, do the verfication including possible side cases.

then it seem appropriate for us to start the disclosure process so other users can start patching.

Comment 16

[Dave Miller [:justdave]]

Fix has been live on our site for a while, we were just waiting to determine if it was still exploitable by any means, and we haven't found anything yet.  opsec should verify, they're better at the xss testcases.

Comment 17

[Dave Miller [:justdave]]

FWIW, this has already been disclosed to most majordomo2 users via the mj2-dev mailing list (which since it never actually got released, the majority of the admins are subscribed to it as the only real source of info about it) on February 5th.    http://comments.gmane.org/gmane.mail.majordomo.majordomo2.devel/4057

Comment 18

[lofi]

(In reply to comment #16)
> opsec should verify, they're better at the xss testcases.

Should i wait for this verification? or is it ok to release the advisory?

Comment 19

[Michael Coates [:mcoates] (acct no longer active)]

I can't find obvious methods of bypassing the regex, but that doesn't provide a lot of assurance since there are so many edge cases.

Let me turn this approach around a bit. Could we specify a whitelist of allowed names for the "extra" parameter? For example [a-zA-Z0-9]?  We could even through in the / character to support subdirectories.

This approach provides a lot more confidence.

Comment 20

[lofi]

my $id = $cgi->param('id');
if ($id) {
    # Be careful not to allow directory traversal.
    if ($id =~ /\.\./) {
        # two dots in a row is bad
        ThrowCodeError("bad_page_cgi_id", { "page_id" => $id });
    }
    # Split into name and ctype.
    $id =~ /^([\w\-\/\.]+)\.(\w+)$/;
    if (!$2) {
        # if this regexp fails to match completely, something bad came in
        ThrowCodeError("bad_page_cgi_id", { "page_id" => $id });
    }

This is how bugzilla is handling the directory traversal problem.

Comment 21

[Michael Coates [:mcoates] (acct no longer active)]

(In reply to comment #20)
I'd argue against a more complex regex. This becomes more difficult to reliably verify. Also, this code appears to be serving a slightly different purpose.

Comment 22

[lofi]

I just posted it, that you can see, that bugzilla use the currently used regex. And it's working.

Comment 23

[lofi]

The Status is RESOLVED FIXED.

Can i release the advisory now?

Comment 24

[Michael Coates [:mcoates] (acct no longer active)]

(In reply to comment #23)
> The Status is RESOLVED FIXED.
> 
> Can i release the advisory now?

We need to mark this as "Verified" before we can publicly release any info.  I'll have this wrapped up by tomorrow (3/8/11)

Comment 25

[Michael Coates [:mcoates] (acct no longer active)]

The solution is not ideal, but we can't identify any vectors to bypass the solution.

Issue verified. Fix in prod and confirmed.  This can be released for advisories.

Comment 26

[Michael Coates [:mcoates] (acct no longer active)]

(In reply to comment #15)
> justdave and cshields,  can you get someone to mark this fixed, do the
> verfication including possible side cases.
> 
> then it seem appropriate for us to start the disclosure process so other users
> can start patching.

Chris,  Who is coordinating the disclosure?

Comment 27

[chris hofmann]

is that a question for me chris, or clyon chris, or other chris? ;-)

seems like someone from majordomo would be the best candidate to coordinate and do the disclosure.

Comment 28

[Michael Coates [:mcoates] (acct no longer active)]

Sorry, question was for you (Chofmann) per your comment 15

Comment 29

[lofi]

(In reply to comment #25)
> The solution is not ideal, but we can't identify any vectors to bypass the
> solution.
> 
> Issue verified. Fix in prod and confirmed.  This can be released for
> advisories.

Ok ... i will send the advisory today to full-disc...

Comment 30

[Dave Miller [:justdave]]

(In reply to comment #27)
> seems like someone from majordomo would be the best candidate to coordinate and
> do the disclosure.

As mentioned in comment 17, it was already disclosed via Majordomo2's channels on February 5th.