glebd / cocoafob

A set of registration code generation and verification helpers for Obj-C, Ruby (Potion Store), PHP and FastSpring
Other
469 stars 57 forks source link

Swift: Problem with extra characters on the registration key #50

Closed trailfog closed 2 years ago

trailfog commented 3 years ago

While playing around, I noticed that adding extra characters to the end of the registration key still produces a valid return in the swift5 implementation. This is down to how the Security framework by apple handles additional characters during a Base32 decode: They simply get ignored and no error is thrown!

Example: The swift implementation will accept both of these registration keys:

You can reproduce this by adding the following test to the swift5 project in Xcode:

    func testVerifyAdditionalCharactersFail() {
      let verifier = LicenseVerifier(publicKeyPEM: publicKeyPEM)
      XCTAssertNotNil(verifier?.pubKey)
      let name = "Joe Bloggs"
      let regKey = "GAWQE-F9AQP-XJCCL-PAFAX-NU5XX-EUG6W-KLT3H-VTEB9-A9KHJ-8DZ5R-DL74G-TU4BN-7ATPY-3N4XB-V4V27-Qasdf"
      let result = verifier?.verify(regKey, forName: name) ?? false
      XCTAssertFalse(result)
    }

The expected result would be that this is an invalid key, thus asserting the result with a false value. However, this test will fail, because the verification function will return a true!

I also tried this in the Python implementation and that "successfully" failed during the Base32 decode -> All okay.

As I don't know the security framework to well, I added the following code to the LicenceVerifier.swift:

      ...
      let decoder = try getDecoder(keyData)
      let signature = try cfTry(.error) { SecTransformExecute(decoder, $0) }

      // reverse the signature to check for truncated data / additional data entered by the user
      let encoder = try getEncoder(signature as! Data)
      let reverseSignature = try cfTry(.error) { SecTransformExecute(encoder, $0) }
      let reverseSignatureString = String(decoding: reverseSignature as! Data, as: UTF8.self).replacingOccurrences(of: "=", with: "")
      if(reverseSignatureString != keyString) { return false }

      let verifier = try getVerifier(self.pubKey, signature: signature as! Data, nameData: nameData)
      let result = try cfTry(.error) { SecTransformExecute(verifier, $0) }
      ...

and a bit further down add this function:

  // MARK: - Helper functions
  fileprivate func getEncoder(_ signature: Data) throws -> SecTransform {
    let encoder = try cfTry(.error) { return SecEncodeTransformCreate(kSecBase32Encoding, $0) }
    let _ = try cfTry(.error) { return SecTransformSetAttribute(encoder, kSecTransformInputAttributeName, signature as CFTypeRef, $0) }
    return encoder
  }

I don't think this is the nicest solution, but it works. Does anyone else know this might be fixed with a cleaner method? Seems like a workaround for something that I have missed!

DivineDominion commented 2 years ago

@fheidenreich Would you like to open a PR for your fixes?

fheidenreich commented 2 years ago

@DivineDominion Yes, just opened a PR for this fix (for reference: https://github.com/glebd/cocoafob/pull/55)

glebd commented 2 years ago

Thank you

sweetppro commented 2 years ago

this bug is also present in the objc version

DivineDominion commented 2 years ago

@sweetppro Could you switch to the .framework building version based on Swift instead of "inlining" the .m/.h files into your project? That might be the best option if you don't want to replicate @fheidenreich's fix in ObjC

sweetppro commented 2 years ago

id prefer not to as it would add unnecessary size to my app