Permalink
Browse files

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.
  • Loading branch information...
1 parent 9aa3015 commit b7dc2438a7f9e1f1b79c2aba149e0509a6bba140 @andymatuschak andymatuschak committed Apr 29, 2012
Showing with 9 additions and 1 deletion.
  1. +9 −1 SUAppcast.m
View
10 SUAppcast.m
@@ -94,7 +94,15 @@ - (void)downloadDidFinish:(NSURLDownload *)aDownload
if (downloadFilename)
{
- document = [[[NSXMLDocument alloc] initWithContentsOfURL:[NSURL fileURLWithPath:downloadFilename] options:0 error:&error] autorelease];
+ NSUInteger options = 0;
+ if (NSAppKitVersionNumber < NSAppKitVersionNumber10_7) {
+ // In order to avoid including external entities when parsing the appcast (a potential security vulnerability; see https://github.com/andymatuschak/Sparkle/issues/169), we ask NSXMLDocument to "tidy" the XML first. This happens to remove these external entities; it wouldn't be a future-proof approach, but it worked in these historical versions of OS X, and we have a more rigorous approach for 10.7+.
+ options = NSXMLDocumentTidyXML;
+ } else {
+ // In 10.7 and later, there's a real option for the behavior we desire.
+ options = NSXMLNodeLoadExternalEntitiesSameOriginOnly;
+ }
+ document = [[[NSXMLDocument alloc] initWithContentsOfURL:[NSURL fileURLWithPath:downloadFilename] options:options error:&error] autorelease];
#if MAC_OS_X_VERSION_MIN_REQUIRED <= MAC_OS_X_VERSION_10_4
[[NSFileManager defaultManager] removeFileAtPath:downloadFilename handler:nil];

0 comments on commit b7dc243

Please sign in to comment.