Skip Menu |
 

This queue is for tickets about the HTTP-Body CPAN distribution.

Report information
The Basics
Id: 88342
Status: open
Priority: 0/
Queue: HTTP-Body

People
Owner: GETTY [...] cpan.org
Requestors: jonathan.dolle [...] groupsquad.com
Cc: CARNIL [...] cpan.org
gregoa [...] cpan.org
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)



Subject: HTTP::Body::Multipart critical security bug
Date: Mon, 02 Sep 2013 17:39:04 +0200
To: bug-HTTP-Body [...] rt.cpan.org
From: Jonathan Dollé <jonathan.dolle [...] groupsquad.com>
Download (untitled) / with headers
text/plain 1.2k
Hello, We discovered a critical bug in HTTP::Body::Multipart >= 1.08. It concerns this point : Temp files now preserve the suffix of the uploaded file. This makes it possible to feed the file directly into a mime-type-determining module that may rely on this suffix as part of its heuristic. (Dave Rolsky) The following line is not good : my $suffix = $basename =~ /[^.]+(\.[^\\\/]+)$/ ? $1 : q{}; It is too much permissive. For example, with the following file name : "2013-06-19 at 11.37.56 PM.png" We can obtain this temp file : "/tmp/k6gvivOIYK.37.56 PM.png" It take everithing after the first dot, even spaces ! Previously, the tempname was always alphanumeric. No special chars. So we could use it directly in commands like: my $info = `identify -format "%m" $filename 2>&1`; With a space, the command become invalid. Worse : we can easily do 'injections'. For example with a filename like: "file. || rm -rf ~ || .png" I recommand the following regexp: my $suffix = $basename =~ /[^.]+(\.[\w]+)$/ ? $1 : q{}; Or, for extension like '.tar.gz': my $suffix = $basename =~ /[^.]+(\.[\w\.]+)$/ ? $1 : q{}; Or better: my $suffix = $basename =~ /[^.]+((?:\.[\w+])+)$/ ? $1 : q{}; Best regards, Jonathan Dolle
Download (untitled) / with headers
text/plain 1.4k
Thanks for your report, will asap check this up and fix it and make a new release. Am Mo 02. Sep 2013, 11:39:22, jonathan.dolle@groupsquad.com schrieb: Show quoted text
> Hello, > > We discovered a critical bug in HTTP::Body::Multipart >= 1.08. > > It concerns this point : > > Temp files now preserve the suffix of the uploaded file. This makes > it possible to feed the file directly into a mime-type-determining > module that may rely on this suffix as part of its heuristic. (Dave > Rolsky) > > > The following line is not good : > my $suffix = $basename =~ /[^.]+(\.[^\\\/]+)$/ ? $1 : q{}; > > > It is too much permissive. > For example, with the following file name : > "2013-06-19 at 11.37.56 PM.png" > > We can obtain this temp file : > "/tmp/k6gvivOIYK.37.56 PM.png" > > It take everithing after the first dot, even spaces ! > > Previously, the tempname was always alphanumeric. No special chars. So > we could use it directly in commands like: > my $info = `identify -format "%m" $filename 2>&1`; > > With a space, the command become invalid. Worse : we can easily do > 'injections'. > For example with a filename like: > "file. || rm -rf ~ || .png" > > I recommand the following regexp: > my $suffix = $basename =~ /[^.]+(\.[\w]+)$/ ? $1 : q{}; > > Or, for extension like '.tar.gz': > my $suffix = $basename =~ /[^.]+(\.[\w\.]+)$/ ? $1 : q{}; > Or better: > my $suffix = $basename =~ /[^.]+((?:\.[\w+])+)$/ ? $1 : q{}; > > > Best regards, > Jonathan Dolle > > > > > > > >
Download (untitled) / with headers
text/plain 201b
Just to say, we are still on it, I involved jnap so that we are overall sure what we do here and that we dont break anything relevant (or at least start giving out a warning if we have to accept this).
Download (untitled) / with headers
text/plain 359b
Hi! On Fri Sep 06 18:32:10 2013, GETTY wrote: Show quoted text
> Just to say, we are still on it, I involved jnap so that we are > overall sure what we do here and that we dont break anything relevant > (or at least start giving out a warning if we have to accept this).
Thanks for that! Do you have any idea when a fix would be available for this issue? Regards, Salvatore
Download (untitled) / with headers
text/plain 533b
Am Di 15. Okt 2013, 11:19:21, CARNIL schrieb: Show quoted text
> Hi! > > On Fri Sep 06 18:32:10 2013, GETTY wrote:
> > Just to say, we are still on it, I involved jnap so that we are > > overall sure what we do here and that we dont break anything relevant > > (or at least start giving out a warning if we have to accept this).
> > Thanks for that! Do you have any idea when a fix would be available > for this issue? > > Regards, > Salvatore
Sorry dropped off scope (stressful time). I will ring the people again and try to push a decision :).
Subject: Re: [rt.cpan.org #88342] HTTP::Body::Multipart critical security bug
Date: Tue, 15 Oct 2013 20:33:35 +0200
To: Torsten Raudssus via RT <bug-HTTP-Body [...] rt.cpan.org>
From: Salvatore Bonaccorso <carnil [...] cpan.org>
Download (untitled) / with headers
text/plain 751b
Hi Torsten, On Tue, Oct 15, 2013 at 01:03:27PM -0400, Torsten Raudssus via RT wrote: Show quoted text
> <URL: https://rt.cpan.org/Ticket/Display.html?id=88342 > > > Am Di 15. Okt 2013, 11:19:21, CARNIL schrieb:
> > Hi! > > > > On Fri Sep 06 18:32:10 2013, GETTY wrote:
> > > Just to say, we are still on it, I involved jnap so that we are > > > overall sure what we do here and that we dont break anything relevant > > > (or at least start giving out a warning if we have to accept this).
> > > > Thanks for that! Do you have any idea when a fix would be available > > for this issue? > > > > Regards, > > Salvatore
> > > Sorry dropped off scope (stressful time). I will ring the people > again and try to push a decision :).
Thanks a lot! Regards, Salvatore
Download (untitled) / with headers
text/plain 310b
Hi Thorsten How would my $suffix = $basename =~ /[^.]++(\.\w+(?:\.\w+)*)$/ ? $1 : q{}; be? Note, it still might be improved, as for example having my $filename = 'C:\\foo\file. || rm -rf ~ || .png'; the above regexp consumes a lot of time backtracking in the first part of the string. Regards, Salvatore
Download (untitled) / with headers
text/plain 506b
Hi Torsten, On Mon Oct 21 05:19:03 2013, CARNIL wrote: Show quoted text
> Hi Torsten > > How would > > my $suffix = $basename =~ /[^.]++(\.\w+(?:\.\w+)*)$/ ? $1 : q{}; > > be? > > Note, it still might be improved, as for example having > > my $filename = 'C:\\foo\file. || rm -rf ~ || .png'; > > the above regexp consumes a lot of time backtracking in the first part > of the string.
Would my $suffix = $basename =~ /(\.\w+(?:\.\w+)*)$/ ? $1 : q{}; even do it better? (Or do I miss something?) Regards, Salvatore
Download (untitled) / with headers
text/plain 1.1k
Sorry for the reply, our problem is just secondary the regexp itself, our problem is more settled that we are scared about the backdraft of changing this default. But jnap just made up a good idea, we will make this configurable as variable (like very simple our $HTTP::Body::tmpregexp) and people can use other regexp, this way we can find out where there are clashs and then settle it after some time with a new default. This way you can safe yourself already asap. Today i am (still) very busy, but i try to sniff in time todo the required updates. Still big thanks for pointing out, just.... stressful time hehe Am Mo 21. Okt 2013, 13:57:36, CARNIL schrieb: Show quoted text
> Hi Torsten, > > On Mon Oct 21 05:19:03 2013, CARNIL wrote:
> > Hi Torsten > > > > How would > > > > my $suffix = $basename =~ /[^.]++(\.\w+(?:\.\w+)*)$/ ? $1 : q{}; > > > > be? > > > > Note, it still might be improved, as for example having > > > > my $filename = 'C:\\foo\file. || rm -rf ~ || .png'; > > > > the above regexp consumes a lot of time backtracking in the first part > > of the string.
> > Would > > my $suffix = $basename =~ /(\.\w+(?:\.\w+)*)$/ ? $1 : q{}; > > even do it better? (Or do I miss something?) > > Regards, > Salvatore
Subject: Re: [rt.cpan.org #88342] HTTP::Body::Multipart critical security bug
Date: Tue, 22 Oct 2013 13:50:07 +0200
To: Torsten Raudssus via RT <bug-HTTP-Body [...] rt.cpan.org>
From: Salvatore Bonaccorso <carnil [...] cpan.org>
Download (untitled) / with headers
text/plain 1.8k
Hi Torsten On Mon, Oct 21, 2013 at 02:30:45PM -0400, Torsten Raudssus via RT wrote: Show quoted text
> <URL: https://rt.cpan.org/Ticket/Display.html?id=88342 > > > Sorry for the reply, our problem is just secondary the regexp > itself, our problem is more settled that we are scared about the > backdraft of changing this default. But jnap just made up a good > idea, we will make this configurable as variable (like very simple > our $HTTP::Body::tmpregexp) and people can use other regexp, this > way we can find out where there are clashs and then settle it after > some time with a new default. This way you can safe yourself already > asap. Today i am (still) very busy, but i try to sniff in time todo > the required updates. Still big thanks for pointing out, just.... > stressful time hehe
Thanks again for you reply and time you invest in resolving the issuse! Regarding the proposed solution: Even in that case I suggest to stick with a 'more secure' default from beginning on if this is feasible. I agree, however that one would need to reuse the temporary filename somewhere else, without further checking/sanitizing. But I wanted to suggest "safe by default", wider rule if needed and developer of depending application know the problem and does additional checks. This should be clearly marked in any case in docs and changes so that people using it are aware of the change. What do you think about this? Side node, could you refer/add a reference to CVE-2013-4407 in your changelog when updating HTTP::Body? This CVE was recently assigned for this issue, and other security teams for distributions might this way better track the update. Some references: [0] http://www.openwall.com/lists/oss-security/2013/10/08/1 [1] https://security-tracker.debian.org/tracker/CVE-2013-4407 [2] https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2013-4407 Wish you a less stressfull time! Regards, Salvatore
Download (untitled) / with headers
text/plain 160b
Hi Torsten Did you had a chance to look at the implementation you mentioned for resolving this issue? (sorry for beeing that anoying ;-)) Regards, Salvatore
Download (untitled) / with headers
text/plain 300b
You are not annoying, we are just lazy and overloaded ;) Its on the eye..... Am Di 29. Okt 2013, 19:16:12, CARNIL schrieb: Show quoted text
> Hi Torsten > > Did you had a chance to look at the implementation you mentioned for > resolving this issue? (sorry for beeing that anoying ;-)) > > Regards, > Salvatore
Download (untitled) / with headers
text/plain 329b
I have also wondered about adding the suffix. Always wary of using user-supplied data like this. An option to disable this feature might be the way to go. I'd also like to see the TEMPLATE for File::Temp be configurable (perhaps a global). For example, I use: our $TEMPFILE_TEMPLATE = "http_body_multipart_$$_XXXXXXXX";
From: felix.ostmann [...] gmail.com
Download (untitled) / with headers
text/plain 3.3k
Am Di 22. Okt 2013, 07:50:30, CARNIL schrieb: Show quoted text
> Hi Torsten > > On Mon, Oct 21, 2013 at 02:30:45PM -0400, Torsten Raudssus via RT wrote:
> > <URL: https://rt.cpan.org/Ticket/Display.html?id=88342 > > > > > Sorry for the reply, our problem is just secondary the regexp > > itself, our problem is more settled that we are scared about the > > backdraft of changing this default. But jnap just made up a good > > idea, we will make this configurable as variable (like very simple > > our $HTTP::Body::tmpregexp) and people can use other regexp, this > > way we can find out where there are clashs and then settle it after > > some time with a new default. This way you can safe yourself already > > asap. Today i am (still) very busy, but i try to sniff in time todo > > the required updates. Still big thanks for pointing out, just.... > > stressful time hehe
> > Thanks again for you reply and time you invest in resolving the > issuse! > > Regarding the proposed solution: Even in that case I suggest to stick > with a 'more secure' default from beginning on if this is feasible. I > agree, however that one would need to reuse the temporary filename > somewhere else, without further checking/sanitizing. But I wanted to > suggest "safe by default", wider rule if needed and developer of > depending application know the problem and does additional checks. > This should be clearly marked in any case in docs and changes so that > people using it are aware of the change. > > What do you think about this? > > Side node, could you refer/add a reference to CVE-2013-4407 in your > changelog when updating HTTP::Body? This CVE was recently assigned for > this issue, and other security teams for distributions might this way > better track the update. > > Some references: > > [0] http://www.openwall.com/lists/oss-security/2013/10/08/1 > [1] https://security-tracker.debian.org/tracker/CVE-2013-4407 > [2] https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2013-4407 > > Wish you a less stressfull time! > > Regards, > Salvatore
I would also prefer a save default and for every one who wants to use it a simple option would be great. additional there is another problem with this feature: Caught exception in engine "Error in tempfile() using template /tmp/XXXXXXXXXX.(#dm=@ognl.OgnlContext@DEFAULT_MEMBER_ACCESS).(#_memberAccess?(#_memberAccess=#dm):((#container=#context['com.opensymphony.xwork2.ActionContext.container']).(#ognlUtil=#container.getInstance(@com.opensymphony.xwork2.ognl.OgnlUtil@class)).(#ognlUtil.getExcludedPackageNames().clear()).(#ognlUtil.getExcludedClasses().clear()).(#context.setMemberAccess(#dm)))).(#matt= #context.get('com.opensymphony.xwork2.dispatcher.HttpServletResponse'),#matt.getWriter().println (: Could not create temp file /tmp/0YTB9thX8o.(#dm=@ognl.OgnlContext@DEFAULT_MEMBER_ACCESS).(#_memberAccess?(#_memberAccess=#dm):((#container=#context['com.opensymphony.xwork2.ActionContext.container']).(#ognlUtil=#container.getInstance(@com.opensymphony.xwork2.ognl.OgnlUtil@class)).(#ognlUtil.getExcludedPackageNames().clear()).(#ognlUtil.getExcludedClasses().clear()).(#context.setMemberAccess(#dm)))).(#matt= #context.get('com.opensymphony.xwork2.dispatcher.HttpServletResponse'),#matt.getWriter().println (: Der Dateiname ist zu lang at /home/domainprofi/www/productparking/root/opt/perl/5.20.1-native/lib/HTTP/Body/MultiPart.pm line 280."


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.