Bug 772787

Summary: able to upload binary data as a system template
Product: [Retired] CloudForms Cloud Engine Reporter: wes hayutin <whayutin>
Component: aeolus-conductorAssignee: Tzu-Mainn Chen <tzumainn>
Status: CLOSED NOTABUG QA Contact: wes hayutin <whayutin>
Severity: low Docs Contact:
Priority: unspecified    
Version: 1.0.0CC: akarol, deltacloud-maint, hbrock, kseifried, ssachdev, tzumainn
Target Milestone: rc   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
URL: https://qeblade31.rhq.lab.eng.bos.redhat.com/conductor/images/edit_xml
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2012-02-06 18:53:45 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Attachments:
Description Flags
ss1
none
ss2 none

Description wes hayutin 2012-01-09 23:38:15 UTC
Created attachment 551714 [details]
ss1

Description of problem:

env -> pool -> new image

maliciously upload any kind of file.. a png, or other binary file.
File does get uploaded...

Wondering if this is a security risk??

See screenshots

Comment 1 wes hayutin 2012-01-09 23:38:57 UTC
Created attachment 551715 [details]
ss2

Comment 2 Tzu-Mainn Chen 2012-01-10 21:47:54 UTC
I'm not entirely sure what the added risk is; from what I can tell (although I may be missing something), it's not possible to tell if a file is binary without reading it, so the upload would have to go through either way. . . ?  Unless there's an alternative process that I'm missing.


Mainn

Comment 3 wes hayutin 2012-01-12 16:34:06 UTC
adding to ce-sprint

Comment 4 wes hayutin 2012-01-12 16:40:27 UTC
removing ce-sprint-next tracker

Comment 5 Hugh Brock 2012-01-24 17:20:40 UTC
Kurt, can we get a security opinion on how severe a problem this is? We're talking about a user's ability to upload a file claiming it is an XML document when it could in fact be anything (and we don't check to see what it is). We would never sent the file to image factory as actual build instructions, because it wouldn't pass XML validation, but it would be in the system.

Thanks,
--Hugh

Comment 6 Kurt Seifried 2012-01-24 22:36:59 UTC
Some possible problems this creates:

1) additional attacks against the XML parser beyond simple XML attacks, e.g. binary data that triggers logging errors/etc.

2) if the files are stored in an accessible location they could be used in conjunction with other attacks, e.g. a local file inclusion flaw in the web interface or something, or local vulns (pass a malformed file to something)

3) Do the files eventually get cleaned out somehow? If not they could result in usage of disk space or inodes.

Comment 7 Tzu-Mainn Chen 2012-01-24 22:49:41 UTC
I agree that it can be a danger, but I don't see how it's possible to analyze the file without actually uploading the file.  Just looking at the extension isn't reliable.  Are we just looking to make sure that a file is deleted if it's not xml, or. . . ?

Comment 8 Kurt Seifried 2012-01-25 17:01:18 UTC
Ideally the file should be examined once uploaded and if not XML deleted and an error thrown back to the user (they may have selected the wrong file, a corrupted file, etc.). Obviously malformed XML can be uploaded (Turing and all that) but at least we avoid the obvious garbage.

Comment 9 Tzu-Mainn Chen 2012-01-25 18:42:42 UTC
We should be fine then - the code that reads the XML is this:

      file = params[:image_file]
      xml_source = file && file.read

As far as I can tell, this doesn't actually save the file; worst case, if the file is over a certain size, it creates a temp file that is removed after the request ends.  Does that satisfy this issue. . . ?

Comment 10 wes hayutin 2012-02-06 18:53:45 UTC
determined not to be a bug..