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!

sonarcloud · 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?

…D4, MD5 for iOS Rework md5 into md5_insecure Provide a SHA1_insecure Update travis.yml

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... 😦

* Don't define "md2" for Linux * Rework deprecation warnings to say "cryptographically insecure" (instead of "broken")

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...)

sonarcloud · 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 force-pushed the Issue77-remove-md2-md4-md5 branch from ce64a36 to 7bc967e Compare November 21, 2024 21:43

[Issue Kitura#77] Obsolete cryptographically broken algorithms MD2, M…

a67f264

…D4, MD5 for iOS Rework md5 into md5_insecure Provide a SHA1_insecure Update travis.yml

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

PR Comments

beffc20

* Don't define "md2" for Linux * Rework deprecation warnings to say "cryptographically insecure" (instead of "broken")

Ensuring no references to .md2 when compiling for Linux

ff8e978

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

LowAmmo commented Sep 30, 2022 •

edited

Loading

LowAmmo commented Feb 28, 2023

mman commented Feb 28, 2023

mman commented Mar 7, 2023

sonarcloud bot commented Mar 28, 2023

LowAmmo commented Nov 19, 2024

dannys42 commented Nov 20, 2024

CLAassistant commented Nov 20, 2024 •

edited

Loading

LowAmmo commented Nov 20, 2024

dannys42 commented Nov 21, 2024

LowAmmo commented Nov 21, 2024

LowAmmo commented Nov 21, 2024

LowAmmo commented Nov 21, 2024

dannys42 left a comment

dannys42 Nov 24, 2024

LowAmmo Dec 3, 2024

dannys42 Nov 24, 2024

LowAmmo Dec 3, 2024

dannys42 Nov 24, 2024

LowAmmo Dec 3, 2024

dannys42 Nov 24, 2024

LowAmmo Dec 3, 2024

dannys42 commented Nov 24, 2024

LowAmmo commented Dec 3, 2024

LowAmmo commented Dec 3, 2024

sonarcloud bot commented Dec 4, 2024

LowAmmo commented Dec 12, 2024

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

Conversation

LowAmmo commented Sep 30, 2022 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Checklist:

LowAmmo commented Feb 28, 2023

mman commented Feb 28, 2023

mman commented Mar 7, 2023

sonarcloud bot commented Mar 28, 2023

LowAmmo commented Nov 19, 2024

dannys42 commented Nov 20, 2024

CLAassistant commented Nov 20, 2024 • edited Loading

LowAmmo commented Nov 20, 2024

dannys42 commented Nov 21, 2024

LowAmmo commented Nov 21, 2024

LowAmmo commented Nov 21, 2024

LowAmmo commented Nov 21, 2024

dannys42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannys42 commented Nov 24, 2024

LowAmmo commented Dec 3, 2024

LowAmmo commented Dec 3, 2024

sonarcloud bot commented Dec 4, 2024

Quality Gate failed

LowAmmo commented Dec 12, 2024

LowAmmo commented Sep 30, 2022 •

edited

Loading

CLAassistant commented Nov 20, 2024 •

edited

Loading