search

pinterest / PINRemoteImage

A thread safe, performant, feature rich image fetcher
Apache License 2.0
4.01k stars 511 forks source link

using deprecated in iOS13 MD5 for file cache keys #582

Open bolsinga opened 4 years ago

bolsinga commented 4 years ago

Added with #189. It is not clear to me from reading that what the issue was, but using this will get deprecation warnings when targeting iOS13.

dyomin-d commented 3 years ago

I face the same security issue. Here you may find the message from Data Theorem:

The vulnerable code locations use the CC_MD hashing functions, which leverage hashing algorithms (including MD2 and MD5) that are proven to be vulnerable to collision attacks, and are unsuitable for modern use.

Apple has officially deprecated these APIs in the iOS 13.0 SDK. They state in the CommonCrypto headers:

"These functions are cryptographically broken and should not be used in security contexts. Clients should migrate to SHA256 (or stronger)."

The following code locations within the App use the deprecated CC_MD functions: -[PINRemoteImageManager cacheKeyForURL:processorKey:resume:] calls _CC_MD5_Init() -[PINRemoteImageManager cacheKeyForURL:processorKey:resume:] calls _CC_MD5_Final() etc.

Please prioritise the issue as it is a serious security downlift.

SAGESSE-CN commented 3 years ago

Please prioritise the issue as it is a serious security downlift.

This will not cause any security issues.

The vulnerable code locations use the CC_MD hashing functions, which leverage hashing algorithms (including MD2 and MD5) that are proven to be vulnerable to collision attacks, and are unsuitable for modern use.

When you use the MD2/MD5 in crypto system world that's true, but we don't, it's not a problem just a warning. cacheKeyForURL does not use MD5 for any encryption.

We do need to do some data digesting of the URL to prevent the name too long.

Suggestion:

  1. Continue with CC_MD5, turn off the warnings before call, since we already know why
  2. Replace with the new Hash algorithm
garrettmoon commented 3 years ago

@sagesse-cn is correct, this probably isn't much of an attack vector (I suppose your app could be tricked into showing an item from the cache when it wasn't supposed to…)

That said, it's deprecated, we should move to a separate hashing algorithm. Sadly this will mean either we'll need to add support for migrate caches for folks so it won't be easy to address.

dyomin-d commented 3 years ago

Thank you for the comment. I hope you will put it in your roadmap as it influences consumer's app security.