343552 - Bonsai shouldn't try to look ouside of cvsroot via ../

Status: RESOLVED FIXED

(Webtools Graveyard :: Bonsai, defect)

Description

[Andrew Schultz]

I saw this in my logwatch mail this morning

!!!! 1 possible successful probes 
 /bonsai/cvsblame.cgi?file=../../../../../../../../../../../etc/passwd HTTP Response 200 

Further testing indicates
1. bonsai gave them an error page ("Invalid filename")
2. It was the wrong path to passwd
3. bonsai wouldn't have delivered the file it if was the right path because it didn't have ",v" on the end.
4. bonsai happily attempts to parse and deliver files (with ,v at the end) outside of /cvsroot.

Comment 1

[Andrew Schultz]

further investigation reveals that ChrootFilename in globals.pl prevents anything bad from happening...  I guess I'd expect that sort of check to happen before it looks for the RCS file.

Comment 2

[Andrew Schultz]

Attachment: like so

This is still not ideal.
file=../some/path/../../cvsroot/somefile
could be used to probe whether /some/path existed or not.

Comment 3

[Andrew Schultz]

Attachment: more robust check

This assumes nothing would hand valid URIs with ../ in them.  If this might happen, canonPath could be taught to die if it tried traverse above cvsroot.

Comment 4

[timeless]

Comment on attachment 228064 [details] [diff] [review]
more robust check

i think tinderbox and friends would use ../'s, certainly i would. let's do this some other way :(

Comment 5

[Andrew Schultz]

Attachment: don't let canonpath walk above cvsroot

Comment 6

[timeless]

Comment on attachment 246702 [details] [diff] [review]
don't let canonpath walk above cvsroot

note that i believe bonsai now requires 5.6 (open calls?). also i'm not a fan of else after return. but this is ok otherwise :).

Comment 7

[Andrew Schultz]

landed.  note that the patch doesn't actually have else after return.  It just looks like it because perl is strange.