Yubico / yubikit-ios

Yubico Mobile iOS SDK - YubiKit
Apache License 2.0
195 stars 43 forks source link

App Crashes due to null value in _Nonnull parameter in YubiKit 4.2.0 #94

Closed WWSellers closed 2 years ago

WWSellers commented 2 years ago

The error argument value can still be nil (null) despite being declared _Nonnull

@optional
- (void)didFailConnectingNFC:(NSError *_Nonnull)error;

Swift version

func didFailConnectingNFC(_ error: Error)

Reproduction:

Workaround Update on workaround: this only works when SWIFT_OPTIMIZATION_LEVEL = "-Onone". The optimizer will remove the code when set to "-O" for release builds.

App must check if error == nil. If it is, then assume that there is no error, just the app calling stopNFCConnection(). However, the Swift compiler gives a warning:

Comparing non-optional value of type 'Error' to 'nil' always returns false

If a developer is treating warnings as errors, then they will need to make exceptions. (And deal with their security departments.)

Recommendation Specifiy _Nullable for the parameter

Other Notes

The YKFNFCConnection member nfcConnectionErroris declared _Nullable,

@property (nonatomic, readonly, nullable) NSError *nfcConnectionError;

but is passed to the delegate (YubiKitManager)

[self.delegate didFailConnectingNFC:self.nfcConnectionError];

YubiKitManager.m implementation does not declare _Nullable or _Nonnull for error

- (void)didFailConnectingNFC:(NSError *)error {

but YubiKitManager.h interface does declare _Nonnull

@optional
- (void)didFailConnectingNFC:(NSError *_Nonnull)error;

(Adding _Nonnull to the implementation did not cause the compiler to give an error)

- (void)didFailConnectingNFC:(NSError *_Nonnull)error {

Environment Xcode 13.3 iPhone 13 with iOS 15.4 YubiKit 4.2.0

jensutbult commented 2 years ago

Yes, this should never happen. The fix in #96 makes sure we only call the didFailConnectingNFC method when there's an actual error to pass on. The reasoning for not passing on an error when stopNFCConnection has been called is that the NFC connection closing without connecting to a YubiKey is expected behaviour at that point.