Potential path-traversal vulnerability in the messaging component of Firebase SDK for iOS

[ztp-mino]

Description

The vulnerability is hypothetical and was found using a source code scanner on an unrelated project using Firebase SDK for iOS.
I have no way to test it. It was previously reported to the security issue tracker and found to be not important enough. So I am reporting it again here.

Affected Branch: main (all since PR #6591)
File: firebase-ios-sdk/FirebaseMessaging/Sources/FIRMessagingExtensionHelper.m

The method "fileExtensionForResponse" (line 140) attempts to construct a proper file extension for image files using the MIME Type received in the response to a network request. This is done by stripping away the "image/" portion of the MIME Type and replacing it with a dot (".").
The constructed extension is later used by simply appending it to a file path (lines 170-173).
There is no check if the MIME Type is sane. If the MIME Type is, for example "image/png/../../../target_file", an unrelated file could be overwritten.

Reproducing the issue

It is unclear how or if this can actually be exploited. An exploit would require downloading of an attachment from a malicious web server.

Firebase SDK Version

main (all since PR #6591)

Xcode Version

N/A

Installation Method

N/A

Firebase Product(s)

Messaging

Targeted Platforms

N/A

Relevant Log Output

If using Swift Package Manager, the project's Package.resolved

If using CocoaPods, the project's Podfile.lock

[google-oss-bot]

I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.

[leojaygoogle]

This isn't likely a security issue. The API in use doesn't allow overwriting files.

firebase-ios-sdk/FirebaseMessaging/Sources/FIRMessagingExtensionHelper.m

Line 173 in deb3fd0

[fileManager moveItemAtURL:temporaryFileLocation toURL:localURL error:&error];

https://developer.apple.com/documentation/foundation/nsfilemanager/1414750-moveitematurl#discussion

If an item with the same name already exists at dstURL, this method stops the move attempt and returns an appropriate error

With that being said, I suspect iOS doesn't actually need the extension to function correctly. I will experiment a little bit and see whether I can remove the fileExtensionForResponse method.

[leojaygoogle]

So, I experimented a little more, and found that:

1. You cannot traverse out of your designated directory
   When you do that (I used https://httpbin.org/response-headers?Content-Type=image//../../test as the image url.), you will get an error like this:

   Failed to move the image file to local location: file:///private/var/mobile/Containers/Data/PluginKitPlugin/xxxxxxxxx-xxxx-xxx-xxxx-xxxxxxxxxxxx/tmp/CFNetworkDownload_weoQxp.tmp./../../test, error: Error Domain=NSCocoaErrorDomain Code=4 "“CFNetworkDownload_weoQxp.tmp” couldn’t be moved to “..” because either the former doesn’t exist, or the folder containing the latter doesn’t exist."

2. Before checking the MIME type, we use response.suggestedFilename.

   firebase-ios-sdk/FirebaseMessaging/Sources/FIRMessagingExtensionHelper.m

   Lines 141 to 148 in deb3fd0

   	NSString *suggestedPathExtension = [response.suggestedFilename pathExtension];
   	if (suggestedPathExtension.length > 0) {
   	return [NSString stringWithFormat:@".%@", suggestedPathExtension];
   	}
   	if ([response.MIMEType containsString:kImagePathPrefix]) {
   	return [response.MIMEType stringByReplacingOccurrencesOfString:kImagePathPrefix
   	withString:@"."];
   	}

   This property is supposed to check the MIME type as well. But it's not clear to me how it's done. https://developer.apple.com/documentation/foundation/nsurlresponse/1415924-suggestedfilename

   In most cases, this property appends the proper file extension based on the MIME type.

Based on the fact that you cannot overwrite existing files or traverse out of your designated directory, I will keep the code as is to avoid potentially breaking SDK users.

Feel free to reopen this issue if you think my analysis is incorrect.

[ztp-mino]

Thank you for looking into this and sorry for the late reply.

I think even just constructing the malicious path could lead to issues further down the road. If the path is also used elsewhere due to a future change, for example.

If you keep the code as is, might I suggest some test cases that check for the behavior you observed in 1.? In case either the code itself or the API changes for some reason in the future.

Thanks again.