Skip to content

Commit

Permalink
Fixes sparkle-project#169: Security Issue in Parsing XML using NSXMLD…
Browse files Browse the repository at this point in the history
…ocument

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
andymatuschak committed Apr 29, 2012
1 parent 9aa3015 commit b7dc243
Showing 1 changed file with 9 additions and 1 deletion.
10 changes: 9 additions & 1 deletion SUAppcast.m
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down

0 comments on commit b7dc243

Please sign in to comment.