Other exceptions

[b1tninja]

py_webauthn/webauthn/webauthn.py

Line 342 in 691ec12

py_webauthn/webauthn/webauthn.py

Line 394 in 691ec12

Would it maybe be safer to use an "else" for this try/except block? That way it is explicit that there was no exception before continuing to parse the signature counts? In case there are some other exceptions...

Or perhaps _verify_signature could return a boolean?

[futureimperfect]

Hi @b1tninja,

Thanks for your feedback!

Do you have a particular example where something unexpected would happen in the current implementation? I'd prefer not to have _verify_signature return a boolean, but rather stick with raising the specific InvalidSignature exception. Also, I think it currently won't suppress any exceptions.

Also, out of curiosity, what would you have happen within the else: block?

[b1tninja]

Basically my fear is not precisely an issue with the implementation here, but with how it may be used by others

I see the specific exceptions caught (that are likely to occur during correct usage, but there is no "catch-all" for scenarios when perhaps a developer mistakenly passes data of an unexpected type or other unforeseen circumstances that result in an exception other than InvalidSignature/NotImplemented. If outer scope ignored such an error, it could perhaps result in continued execution / return value that might cause the signature to appear valid.

What I was proposing was to put the additional validation inside of the else block as a way of only running code when no exception took place. If at all possible perhaps consider avoiding the nested try/excepts and multiple return points for this core function? Just to make it super clear which conditions can result in a valid signature. I'll follow up with a edit in the morning to make this more clear-- but feel free to close issue, a comment would've sufficed not claiming there is a vulnerability or anything like that.

[futureimperfect]

Thanks for the clarification, @b1tninja. Currently, I'd prefer to keep the signature verification flow similar to how cryptography works, in that it will raise an InvalidSignature exception unless the signature is valid. The idea being that a user of the API may do something like the following.

    try:
        webauthn_credential = webauthn_registration_response.verify()
    except Exception as e:
        return jsonify({'fail': 'Registration failed. Error: {}'.format(e)})

This way, even though they've essentially "caught all" in the except Exception as e line, they can still return an error. If they instead only catch the InvalidSignature, we'll raise any other exception, (e.g., TypeError, ValueError, whatever). They could of course still shoot themselves in the foot by doing something like the following:

    try:
        webauthn_registration_response.verify()
    except:
        // Keep going, assuming everything worked.

There are also ways to do similar bad things, even when returning a boolean, (e.g., by forgetting to check the return value). I personally think this is a bigger risk, but I'm open to hearing other suggestions.

Thanks again for sharing your feedback! I'll go ahead and close for now, but feel free to reopen if you think there's something more we can do.

[raylu]

right now, all the exceptions directly subclass Exception

it'd be much nicer if they shared a common WebAuthnException class or something so we could distinguish them from other errors

[MasterKale]

right now, all the exceptions directly subclass Exception

it'd be much nicer if they shared a common WebAuthnException class or something so we could distinguish them from other errors

@raylu This is a good suggestion, but can you please open a new issue to suggest it? This issue is five years old and closed, not the best place for ideas like yours if I'm to have any chance to remember them.

[raylu]

oh, do you not like re-opening old issues?

filed #214