Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Security Issue in Parsing XML using NSXMLDocument #169

Closed
prathaps opened this Issue · 3 comments

2 participants

@prathaps

Hi Guys,

There is a security issue in the way Sparkle parses the xml. In the SUAppCast.m, in the:

  • (void)downloadDidFinish:(NSURLDownload *)aDownload { ....

document = [[[NSXMLDocument alloc] initWithContentsOfURL:[NSURL fileURLWithPath:downloadFilename] options:0 error:&error] autorelease];
...
}

If the xml file being parsed contains a reference to an external entity, that entity would be parsed. For instance take this simple xml file:

<?xml version="1.0"?>
<!DOCTYPE note [
<!ELEMENT note (to,from,heading,body)>
<!ELEMENT to (#PCDATA)>
<!ELEMENT from (#PCDATA)>
<!ELEMENT heading (#PCDATA)>
<!ELEMENT body (#PCDATA)>

<!ENTITY someEntity SYSTEM "/Users/psridharan/entitytext.txt">
]>

Tove
Jani
Reminder
Don't forget me this weekend!+&someEntity;

notice that someEntity is an external entity pointing to some file on disc. This file could be dangerous. In this case it is simply a text file. But the above code that parses the xml document would subsititute the contents of the file when parsing the element.

Please see details of this security risk in:

http://www.securityfocus.com/archive/1/297714/2002-10-27/2002-11-02/0

Instead if you use:

document = [[[NSXMLDocument alloc] initWithContentsOfURL:[NSURL fileURLWithPath:downloadFilename] options: options:NSXMLDocumentTidyXML error:&error] autorelease];

Notice the use of 'options:NSXMLDocumentTidyXML' instead of 'options:0', what you get after parsing is:

<!DOCTYPE note>
ToveJaniReminderDon't forget me this weekend!+&someEntity;

That is the external entity name has be quoted and hasn't been resolved.

My questions are:

1) Are you going to address this in your next patch? If so, when will it be released?

2) If not, can you please provide a way to make this change in the sparkle 1.5 without having to recompile sparkle? That is can we override some class in the application that is linking with sparkle framework to enable this secure xml parsing?

Thanks,

Prathap

@andymatuschak

I am told that one should not use NSXMLDocumentTidyXML to ensure that these entities are stripped (it's essentially just luck that they are stripped out by the tidier), but that NSXMLParser will take care of this situation reliably. I will try to make time in the next few weeks to rewrite the appcast parsing implementation on top of NSXMLParser, but if it gets much closer to WWDC, I'll be underwater.

@prathaps

Andy,

Thank you for your reply. It would be awesome if you can use NSXMLParser to patch the security hole. Please let me know when this is available. I will get back in touch with you in a few weeks!

Prathap

@andymatuschak andymatuschak closed this issue from a commit
@andymatuschak andymatuschak Fixes #169: Security Issue in Parsing XML using NSXMLDocument
External entities are no longer parsed in appcasts: pre-10.7, we use the
"tidy" option for XML document parsing, which happens to behave this way,
and in 10.7+ we use a new explicit option for this behavior.
b7dc243
@andymatuschak

Using NSXMLParser would have been more work than I could spare time for in the near future, but I think that's okay: while we are mostly just getting lucky that NSXMLDocumentTidyXML happens to strip out these entities, and that would not be a future-proof approach, there's a new NSXMLDocument option for 10.7+ (NSXMLNodeLoadExternalEntitiesSameOriginOnly) which we can use going forward. Old OS versions that happened to behave in the way we want will continue to behave that way.

@iloveitaly iloveitaly referenced this issue from a commit in iloveitaly/Sparkle
@iloveitaly iloveitaly Merge branch 'andymatuschak/master'
* andymatuschak/master: (170 commits)
  Adding Thai localizations to the project.
  Add Thai localization
  Update to the Brazilian Portuguese localization from Victor Figueriedo
  Removed Japanese localization of password prompt from the Xcode project
  Allow the user to try reentering his password if authentication fails.
  Removing Japanese localization of the password prompt, since I changed the design
  Delegated password prompting to the update driver.
  Use NSFileManager interface for DMG unarchive only for 10.7+. On 10.6,  [NSFileManager copyItemAtPath:toPath:error:] can fail with "Argument list too long" if the app bundle contains too many files. The switch to NSFileManager was only required for 10.7 anyway, 10.6 always worked fine.
  Removed changes to the project.pbxproj that were unrelated to the password prompt
  Redesigned password prompt UI
  Removed methods from SUPasswordPrompt.h that didn't need to be exposed there
  Fixes #44: maximumSystemVersion key
  remove the ASW tags since we're submitting this as a pull request to andy
  Updating Danish localization courtesy Daniel Østergaard Nielsen
  support for encrypted disk images
  Fixes #175: Bug: update alert text collides with automatic download checkbox
  Fixes #174: Bug: sparkle:shortVersionString ignored for non-enclosure items
  Fixes #170: An environment variable set by Sparkle
  Fixing some new Clang warnings from Xcode 4.4
  Fixes #169: Security Issue in Parsing XML using NSXMLDocument
  ...

Conflicts:
	SUUIBasedUpdateDriver.m
ee6fece
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.