AndyQ / NFCPassportReader

NFCPassportReader for iOS 13
MIT License
733 stars 235 forks source link

OpenSSL 3.1.5001 #217

Open AndyQ opened 3 months ago

AndyQ commented 3 months ago

I've added a branch - OpenSSL3 which is the start of upgrading to OpenSSL 3.1.5001 (current version).

It works only on BAC at the moment, for some reason (unknown at the moment), PACE fails in step 4 when sending the auth token to the passport (invalid parameters received).

If anyone fancies taking a look and seeing if they can see what I've missed that would be great!

Note - this branch has only the bare minimum code changes to get it compiling. There has been a LOT of changes between OpenSSL1 and 3 - lots of functions have been deprecated (which I need to fix and figure out the alternatives - I think its just a migation from the low level functions to the higher level ECP_PKEY ones - as per the migration guide https://www.openssl.org/docs/man3.0/man7/migration_guide.html)

petersulaf commented 2 months ago

Hi, has anyone figured this out? Upgrading do OpenSSL3 is inevitable because of the strict apple policies (privacy manifest, signing, bitcode etc...)

Thormeard commented 3 weeks ago

During the development one of the pods was statically including OpenSSL3 and forcing passport scanner to also use OpenSSL3. And a problem is that if your passport have certificates with ECDSA - then you have an error during verification:

error 94 at 1 depth lookup: Certificate public key has explicit ECC parameters

And according to ICAO all passports with ECDSA MUST use explicit curve parameters paragraph 4.1.6.3.

Apparently OpenSSL is not allowing to use explicit cure parameters in v3. There is a lot of comments about that (and openssl responded with something like "you should change your ICAO standards").

Note that certificates that are using RSA is still parsed/verified no problem.

Just another thing to keep in mind/test if you are going to update to v3. Would be happy to help/test with the passports I have.

thomzon commented 1 week ago

Hi @AndyQ !

First, thanks for your great work on this, it's very much appreciated.

I'm in the process of trying to migrate to OpenSSL 3. I've made locally pretty much the same as you did on your branch, i.e. the bare minimum to make it compile after switching to OpenSSL 3. I have the same PACE failure.

I'm going to look into this, and will share if I find a solution, but I'm wondering: maybe this issue shouldn't really be tackled by trying to fix the current code, but by rewriting it using non-deprecated APIs? Or is the issue such that it shouldn't make a difference?

thomzon commented 1 week ago

I can't reproduce the issue that @Thormeard is describing, on my side it seems like a different one. It fails like for you @AndyQ when sending the last general auth command.

I've looked into it more and comparing the authentication token generation between the previous version and the OpenSSL 3 version. It seems the AES CMAC generation yields different results with the same inputs between both version, and I think this causes the error. Somehow the CMAC API (which is now deprecated) has changed and produces something different.

Rather than trying to fix it, I'm trying to re-implement the AES CMAC generation by using the EVP_MAC API, as recommended in the OpenSSL 3 migration guide. Unfortunately I'm a bit stuck, the EVP_MAC_init function always fail for me, after trying all the configurations I could think of. Here is the current code:

guard let lib = OSSL_LIB_CTX_new()
else {
    return []
}
defer { OSSL_LIB_CTX_free(lib) }

guard let macAlgo = EVP_MAC_fetch(lib, "CMAC", nil)
else {
    return []
}
defer { EVP_MAC_free(macAlgo) }

guard let ctx = EVP_MAC_CTX_new(macAlgo)
else {
    return []
}
defer { EVP_MAC_CTX_free(ctx) }

var params: [OSSL_PARAM] = []
var cipherString = "aes-128-cbc".cString(using: .utf8)!
let cipherParam = OSSL_PARAM_construct_utf8_string(
    OSSL_MAC_PARAM_CIPHER,
    &cipherString,
    0
)
params.append(cipherParam)
params.append(OSSL_PARAM_construct_end())
var keyValue = key
guard EVP_MAC_init(ctx, &keyValue, key.count, params) == 1
else {
    return []
}
...

Please note it's a draft and I'm hardcoding the cipher as in my test it's always this one, but of course it should be adapted to throw instead of returning an empty array, and to have a dynamic cipher depending on the key size. If anyone can see at a glance what I'm doing wrong, please correct me.

AndyQ commented 1 week ago

I agree we should be moving away from the deprecated functions - thats a lot more work than I currently have time for at the moment sadly!

I'll take a look at your code above when I get a chance and see if I can see anything too.