Hello,
Am 11. Dezember 2006 18:51 schrieb Javier Fernández-Sanguino Peña:
> On Mon, Dec 11, 2006 at 04:57:30PM +0100, Christian Boltz wrote:
[please CC me in replies, I'm not subscribed]
> > it's easy to do some code injection in packages.debian.org:
>
> This is not code injection, it's cross site-scripting. Given that:
OK, maybe I used the wrong name - but cross site-scripting would imply
that you need some scripting. In this case, it's a pure injection via a
link, without any need for scripts ;-)
> - packages.debian.org does not have any kind of client authentication
> - packages.debian.org does not use SSL certificate
>
> this is as much a problem as somebody being able to setup a "fake"
> packages.debian.org or do MITM injection.
No.
It's much easier to setup a link in a webpage than to setup a fake
server, spread wrong DNS information etc.
> Not that I wouldn't want to see this fixed but, really, this is as
> low risk as it can get. Through XSS no one could retrieve user
> credentials and no one should be trusting (in this day an age) the
> information from a website that is not signed (through an SSL
> server-side certificatE).
*LoL*
The people download packages from there which are installed as root
(pre-/postinstall scripts) and might contain harmful binaries.
Therefore a wrong md5sum is not "low risk"!
Oh, and SSL would not change anything here. One can still create a link
with a wrong md5sum to https://packages.debian.org, this time fully
trusted ;-)
> That being said. I've developed a fix for the download CGI
> application (attached). And will submit this as a bug.
--- download.pl 1 Dec 2006 08:42:27 -0000 1.27
+++ download.pl 11 Dec 2006 17:49:34 -0000
@@ -182,17 +182,28 @@
$file = $input->param('file');
param_error( "file" ) unless defined $file;
+# Make file fit in a regexp
+param_invalid ("file") if $file !~ /^[\w\%\.\_\-]+$/;
BTW: Inside [...], you do _not_ need to escape special chars. Your
regex will allow backslashes in the filename - not necessarily what
you want.
And it allows other funny things like
http://packages.debian.org/cgi-bin/download.pl?arch=all&file=..%2F..%2F..%2Finsecure%2Fscript%2Fhacked.deb&md5sum=totally_broken_by__security_team&arch=esel&type=main
or
http://packages.debian.org/cgi-bin/download.pl?arch=i386&file=pool%2Fmain%2Fi%2Fie%2Fmicrosoft_internet_explorer_7.0-3_i386.deb&md5sum=totally_broken_by__security_team&arch=i386&type=main
(hmm, does this package's license meet your strict requirements? ;-)
$md5sum = $input->param('md5sum');
param_error( "md5sum" ) unless defined $md5sum;
+# Make md5sum fit in a regexp
+param_invalid ("md5sum") if $md5sum !~ /^\w{32}$/;
Oh yes, very funny ;-)
http://packages.debian.org/cgi-bin/download.pl?arch=i386&file=pool%2Fmain%2Fd%2Fdietlibc%2Fdietlibc_0.28-3_i386.deb&md5sum=debian_security_team_in_da_house&arch=i386&type=main
(thanks to fefe again)
Again: It is not an option to check the md5sum with a regex, you have
to display the _correct_ md5sum. This means you should not pass it as
URL parameter, but read it from a file or database.
$type = $input->param('type');
param_error( "type" ) unless defined $type;
+# Make type fit in a regexp
+param_invalid ("type") if $type !~ /^\w{1,10}$/;
The param name sounds like you should verify against a fixed list
(array) of allowed values.
$arch = $input->param('arch');
param_error( "arch" ) unless defined $arch;
+# Make arch fit in a regexp
+param_invalid ("arch") if $arch !~ /^[\w\-]{1,10}$/;
+# And also check that it is in the list of supported archs
+param_invalid ("arch") if ! defined ($arches{$arch});
arch is also something that should be checked against a fixed array.
Regards,
Christian Boltz
--
"Never surf faster, than your guardian penguin can fly!"
