Removed support for cryptographically broken algorithms MD2, MD4, MD5 #78

LowAmmo · 2022-09-30T21:30:36Z

Addresses the deprecation that has been there since iOS 13

Also added a project file to the repo to make future maintenance easier

Description

Removing all the code supporting MD2, MD4, MD5
- They have been deprecated by Apple for being cryptographically broken
- Cleans up warnings seen by anyone targeting an iOS version >13.0
Also included a project file to make future updates & testing easier

Motivation and Context

Issue: #77

Trying to eliminate warnings we see in our application (from consuming SwiftJWT, which consumes this)

How Has This Been Tested?

Unit testing in the repo/code. Also verified passivity in our application.

Checklist:

I have submitted a CLA form
(as on other PRs, submitted one electronically, so, assuming that would apply to this, too?)
If applicable, I have updated the documentation accordingly.
If applicable, I have added tests to cover my changes.

LowAmmo · 2023-02-28T18:03:06Z

@dannys42, @mman - Modified the PR to clean things up and match the other related repos that just rely on loading the Package.swift to dynamically generate the xcode project, etc.

mman · 2023-02-28T18:21:22Z

@LowAmmo I took a quick look and it LGTM. I will have to get some time to pull your branch and see whether the compilation will be clean... thanks!

mman · 2023-03-07T16:25:59Z

@dannys42, @mman - Modified the PR to clean things up and match the other related repos that just rely on loading the Package.swift to dynamically generate the xcode project, etc.

Tried your fork in my project and it builds cleanly now! LGTM!

sonarqubecloud · 2023-03-28T23:17:00Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

LowAmmo · 2024-11-19T16:18:39Z

@dannys42 - I think this would be the last one (other than possibly updating to reference to proper dependency versions from Swift-JWT, depending on what versions everything gets released to cocoapods, under).

Thanks...in advance... 😄

dannys42 · 2024-11-20T04:14:56Z

@LowAmmo Thanks for the PR and apologies for the delay. I just want to confirm whether errors/warnings are visible in recent versions of Xcode/Swift? I'm running Xcode 16.1 and not seeing any warnings/errors for macOS or iOS builds.

I understand that these hash algorithms are not cryptographically secure, however they are still hashing functions that may be used in some legacy cases and perfectly fine (e.g. for data integrity checks). As a library that other applications may depend on, I'm hesitant to remove them unless they're causing a problem or difficult to maintain.

CLAassistant · 2024-11-20T16:00:25Z

All committers have signed the CLA.

LowAmmo · 2024-11-20T16:44:09Z

@dannys42 - No worries!

To see the warnings, just need to consume this library in an iOS app that targets iOS 13 or newer as the minimum iOS version.

My rationale was that if we know they are cryptographically broken...probably no one should be using them anymore (so they should be able to not upgrade to the new version of this library). But if you think there are consumers that would want to still use those methods, I could modify this change to consider those erroneous options if the targeted iOS version is 13 or greater.

dannys42 · 2024-11-21T01:08:52Z

@LowAmmo Interesting. I wonder why those warnings don't show when the SPM is targeting iOS in Xcode.

If the underlying calls are deprecated, then I think it would be prudent to annotate those options as deprecated (if possible). It looks like Apple took a different approach and is labeling the hashes as insecure in code. Insecure.MD5. While that's an API breaking change, I think that's a reasonable step as well.

Either way, it wouldn't hurt to update the inline comment stating that these methods are insecure for cryptographic purposes.

For reference, here's what Synk has to say:

MD5 isn't all bad
While the usage of MD5 in security and cryptography is depreciated, it can still be used as a hash algorithm in hash map data structures for example. So, just because you see MD5 being used, doesn’t necessarily mean it is an insecure implementation. It depends on the usage.

Also probably worth mentioning that Amazon S3 ETags may also use MD5 hashes.

LowAmmo · 2024-11-21T18:38:29Z

In that case...I'll see what I can get away with so that properly using everything doesn't produce any warnings in code...

Thinking that mimicking what Apple did makes sense. Deprecate "md5", but add a new value "md5_insecure" to try to highlight the same thing - that it's cryptographically busted, but could still be useful for hashmaps, etc.

LowAmmo · 2024-11-21T20:22:28Z

@dannys42 - I think I have a solution that meets your requests.

Working on cleaning up the code...

Pretty much starting from scratch.

Wasn't sure what you would like the "new" version to be - 3.0.0, 2.0.3, 2.1.0?
(2.1.0 is probably most correct, since these changes are all passive now, but that would require a change to other libraries to consume the new version, etc. Which I am definitely not opposed to get PRs out there for, just want to make sure we do a strategy that works for everyone)

LowAmmo · 2024-11-21T21:50:11Z

@dannys42 - So to try to still force developers to update, I obsoleted md2, md4, & md5 for iOS. But, created an md5_insecure as an alternative.

But, this way they are still available to anyone targeting earlier iOS versions (just have to consume slightly differently), or anyone not doing iOS.

Also cleaned up preprocessor directives so that it should be easier to add support for things like VisionOS, etc., in the future.

Hopefully, this is more what you're looking for?

dannys42

Wow, I'm sorry I didn't realize there would be so much code change. I'll have to look at the unit tests and such later, but overall I think your changes look pretty reasonable.

I had a few thoughts in some areas in case you had some ideas, but if not that's ok. Need to go too crazy.

dannys42 · 2024-11-24T05:20:53Z

Sources/Cryptor/Digest.swift

@@ -51,17 +54,28 @@ public class Digest: Updatable {
    public enum Algorithm {

        /// Message Digest 2 See: http://en.wikipedia.org/wiki/MD2_(cryptography)
+        @available(iOS, introduced: 2.0, obsoleted: 13.0, message: "This function is cryptographically broken and should not be used in security contexts. Clients should migrate to SHA256 (or stronger).")


nitpick: Personally I'm not fond of the word "broken" because it implies that it doesn't work. But I understand it is a common industry phrasing. I like Apple's description of just plainly saying it is not cryptographically secure. (Some more detailed discussion on the topic if you're interested: https://www.quora.com/What-is-broken-cryptography)

But I'm ok with leaving your comment here as is.

Updated to say "insecure" instead of "broken"

dannys42 · 2024-11-24T05:23:07Z

Sources/Cryptor/Digest.swift

        case md5

+        /// Message Digest 5
+        /// - NOTE: Do NOT use for cryptography, considered insecure
+        case md5_insecure


If you're adding _insecure for md5 and sha1, why not md2 and md4?

It'd be cool if there was a way to group the insecure methods under ".insecure.md5"... but I'm not sure that could be done as an enum. :-/

Just because Apple has deprecated md2 & md4 & md5 -which causes build warnings. But, as you pointed out (and I was able to implement), they have the "Insecure" wrapper around MD5 - that doesn't generate warnings, and is still available.

So, just trying to represent what Apple is representing - that consumers shouldn't use md2, md4, or md5 for secure things, but if they want to, they can MD5 for Insecure operations.

dannys42 · 2024-11-24T05:23:57Z

Sources/Cryptor/Digest.swift

-			#if os(macOS) || os(iOS) || os(tvOS) || os(watchOS)
-	            engine = DigestEngineCC<CC_MD2_CTX>(initializer:CC_MD2_Init, updater:CC_MD2_Update, finalizer:CC_MD2_Final, length:CC_MD2_DIGEST_LENGTH)
-			#elseif os(Linux)
+			#if os(Linux)
 				fatalError("MD2 digest not supported by OpenSSL")


Would it be better to exclude md2 as a case in the enum for Linux?

Sure, good idea!

Excluding the definition for md2 from the enum if building for Linux.

dannys42 · 2024-11-24T05:25:13Z

Sources/Cryptor/Digest.swift

+                self.engine = DigestEngineCC<MD4_CTX>(initializer:MD4_Init, updater:MD4_Update, finalizer:MD4_Final, length:MD4_DIGEST_LENGTH)
+            #elseif os(iOS)
+                if #available(iOS 13, *) {
+                    fatalError("MD4 digest is cryptographically broken")


Seems like the failure here is more that MD4 is not supported on iOS 13 and above, not that it is insecure?

It is still supported (as far as I can tell), it just generates a build warning (guessing that may mean that soon it will stop being supported).

dannys42 · 2024-11-24T05:38:28Z

Wasn't sure what you would like the "new" version to be - 3.0.0, 2.0.3, 2.1.0?
(2.1.0 is probably most correct, since these changes are all passive now, but that would require a change to other libraries to consume the new version, etc. Which I am definitely not opposed to get PRs out there for, just want to make sure we do a strategy that works for everyone)

Thanks for trying to keep the API backward compatible. If we're following semver, then yes I think this qualifies as a minor version bump (i.e. 2.1.0).

Do the other libraries actually require these changes? It's probably safe leaving them at "from: 2.0.x", since "update packages" should resolve to the latest in the 2.x.y line, no?

LowAmmo · 2024-12-03T15:20:17Z

@dannys42 - Not sure on SPM, but with the way Cocoapods works, Swift-JWT pulls in v. 2.0.1 -> 2.0.999
https://github.com/Kitura/Swift-JWT/blob/master/SwiftJWT.podspec#L17

Using the pessimistic operator. So, to pull in a 2.1.0, we would have to make a PR on Swift-JWT to either change to "2.1" (to accept any future minor versions also), or "2.1.0" (to accept any future defect version fixes on that minor version). Could also change to ">="...but that would make any future major API changes pretty tough to do.

If the offer still stands, I'll gladly take on the role of maintaining the cocoapod repo for all of these libraries (BlueRSA, BlueCryptor, SwiftJWT, etc.), since I know it's a dying system...but...we won't be switching off it anytime soon... 😦

LowAmmo · 2024-12-03T15:42:05Z

@dannys42 - Looks like SonarCloud is now running to test the builds... Any idea of how to mark the "hotspots" as safe? (since they are used expressly for the insecure support cases?) (not sure I have the privileges/ability to mark them...)

sonarqubecloud · 2024-12-04T13:03:39Z

Quality Gate failed

Failed conditions
2 Security Hotspots

See analysis details on SonarQube Cloud

LowAmmo · 2024-12-12T23:27:24Z

@dannys42 - Guessing whatever little freetime you had in early November is busy again (or you're possible taking the "use it or lose it" December PTO... 😉 ). But, just hoping pull in some of these PRs sometime soon (and possibly getting things published to cocoapods) is still on your radar! 😄

-Thanks!

LowAmmo · 2025-04-03T22:29:10Z

@dannys42 - Similar ping on this PR...

(I know, I know...shouldn't look a gift horse in the mouth... 😉 )

dannys42 · 2025-04-05T04:56:11Z

@LowAmmo No worries, ping as needed. I'm sorry for not being more on top of things.

To be frank, this PR has a lot more going on, so it's a bit more for me to evaluate. I think there's a number of goals here, if I'm reading things correctly:

Intentionally breaking the API for insecure hashes
Fixing build pipeline issues
Fixing documentation
Supporting newer Apple platforms

Maybe some of these are inter-related and unavoidable, but smaller PRs would be much easier to digest. It does look like there's a fair amount of existing complexity in supporting Linux vs Apple platforms, and some of the changes look like it's just moving some things around in the #ifdef's, which I understand... it just gets hard to read with all the other changes.

On the topic of the insecure hashes...

I think I'm still just weirded out about intentionally breaking APIs to mark them as insecure
Runtime crashes don't seem like an ideal mitigation for unsupported algorithms. I'd much prefer some compile-time approach to ensuring unsupported algorithms are not possible (e.g. #ifdefs around the enums).. it looks like maybe there's an attempt at that already, but the compiler is forcing you into fatalError() cases that can never happen? (a comment to that affect would help)
It looks like you went through some hoops to deal with algorithms that are no longer supported by either Apple or OpenSSL (thank you for your diligence!). For those algorithms in particular, I think it's safe to just deprecate entirely.
Just a thought... if we bump up the minimum versions to Swift 5.7, I think that would force the minimum iOS to 15. In this case, we might simplify some of the logic. We'd have to make this a major version bump release. But if you think it'll simplify the code, maybe worth investigating. But if it changes things too much, then maybe it's better for that to be a future PR.

LowAmmo · 2025-04-07T15:49:16Z

@dannys42 - I agree...also a fan of lots of small PRs versus one big one, but was trying to make your life easier with just 1 PR.

I can definitely look at breaking these changes into a few different PRs.

If you're good setting the minimum iOS version to 15, I can definitely work at that (all projects that pull this in would end up needing to also update to the same minimum, just as a warning - so, SwiftJWT would need to update to iOS 15).

The runtime errors are more of a way to get around the different compile time warnings/issues across the different platforms. I can examine a little more and see if there might be different ways to do it. (And/or provide some comments in the code to call out the rationale).

It's been a little while since I made these changes...so I'll have to dive back in, but then I can add some comments to the code, too.

So, my TODO's for this PR:

Break into 2-3 separate PRs
Add some additional code comments to clarify why certain things are being done
- Make sure to favor compile time failures over runtime failures
Just remove the no-longer supported algorithms
Set the minimum iOS version to 15 (and correlating mac, tv, and watch versions)
Set version to 2.1.0
Create a PR for SwiftJWT to update accordingly

Let me know if I misunderstood anything!

FYI - without actually looking into it (like it might end up too confusing)...I'll try to keep this PR around as one of the remaining PRs (just with less total changes being done), so we can more easily maintain the history of all of these discussions/comments.

-Thanks!

…sary files * Modify all of the os() compiler directives to just make an exception for linux, but group other platforms together * Will make supporting iPadOS & VisionOS easier in the future * Specify platform versions in Package.swift to match those in cocoapods

* Address the deprecation warning due to Apple considering them insecure * Update platform versions (cocoapods / SPM): * MacOS - 11 / 11.5 * iOS - 14 / 14.5 * tvOS - 14 / 14.5 * watchOS - 7 / 7.5 * Update Swift Version to 5.4 * Keep md4 & md5 for Linux * Add md5_insecure & sha1_insecure to mimic the Apple APIs

sonarqubecloud · 2025-04-08T21:54:15Z

Quality Gate failed

Failed conditions
2 Security Hotspots
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

LowAmmo · 2025-04-08T21:56:53Z

@dannys42 - I update this Branch to go on top of PR #82 ... but looks like github won't let me change the base branch of this PR to my other branch (because it is in my personal repo).

So...once you pull #82, then looking at this will be much easier - only showing the few changes.

(when reviewing this one, you can also look at JUST the 1 commit to see the changes that are actually impactful).

Let me know if these changes work for you - hopefully a lot easier to review, at least...

-Thanks!

LowAmmo · 2025-04-08T22:29:46Z

@dannys42 - Also, sonarcube has some false negatives that can be ignored / cleaned up.

LowAmmo force-pushed the Issue77-remove-md2-md4-md5 branch from ce64a36 to 7bc967e Compare November 21, 2024 21:43

LowAmmo force-pushed the Issue77-remove-md2-md4-md5 branch from 328609a to a67f264 Compare November 21, 2024 22:36

dannys42 reviewed Nov 24, 2024

View reviewed changes

LowAmmo added 2 commits April 8, 2025 10:58

LowAmmo mentioned this pull request Apr 8, 2025

[Issue #77] Clean up platform specific logic and remove unnecessary f… #82

Open

3 tasks

LowAmmo force-pushed the Issue77-remove-md2-md4-md5 branch from ff8e978 to 8da0317 Compare April 8, 2025 21:53

LowAmmo mentioned this pull request Apr 8, 2025

Update Platform versions & dependency version for Cryptor & CryptorRSA Kitura/Swift-JWT#130

Open

Removed support for cryptographically broken algorithms MD2, MD4, MD5 #78

Are you sure you want to change the base?

Removed support for cryptographically broken algorithms MD2, MD4, MD5 #78

Uh oh!

Conversation

LowAmmo commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Checklist:

Uh oh!

LowAmmo commented Feb 28, 2023

Uh oh!

mman commented Feb 28, 2023

Uh oh!

mman commented Mar 7, 2023

Uh oh!

sonarqubecloud bot commented Mar 28, 2023

Uh oh!

LowAmmo commented Nov 19, 2024

Uh oh!

dannys42 commented Nov 20, 2024

Uh oh!

CLAassistant commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LowAmmo commented Nov 20, 2024

Uh oh!

dannys42 commented Nov 21, 2024

Uh oh!

LowAmmo commented Nov 21, 2024

Uh oh!

LowAmmo commented Nov 21, 2024

Uh oh!

LowAmmo commented Nov 21, 2024

Uh oh!

dannys42 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dannys42 commented Nov 24, 2024

Uh oh!

LowAmmo commented Dec 3, 2024

Uh oh!

LowAmmo commented Dec 3, 2024

Uh oh!

sonarqubecloud bot commented Dec 4, 2024

Quality Gate failed

Uh oh!

LowAmmo commented Dec 12, 2024

Uh oh!

LowAmmo commented Apr 3, 2025

Uh oh!

dannys42 commented Apr 5, 2025

Uh oh!

LowAmmo commented Apr 7, 2025

Uh oh!

sonarqubecloud bot commented Apr 8, 2025

Quality Gate failed

Uh oh!

LowAmmo commented Apr 8, 2025

Uh oh!

LowAmmo commented Apr 8, 2025

Uh oh!

LowAmmo commented Sep 30, 2022 •

edited

Loading

CLAassistant commented Nov 20, 2024 •

edited

Loading