Exception handling in u2flib

[pelme]

Calling verify_authenticate / complete_register can raise a bunch of different exceptions.

Here are some examples:

>>> from u2flib_server.u2f_v2 import start_authenticate, complete_register
>>> start_authenticate({})
Traceback (most recent call last):
  File "<input>", line 1, in <module>
  File "/Users/andreas/code/x/venv-2.7/lib/python2.7/site-packages/u2flib_server/u2f_v2.py", line 228, in start_authenticate
    appId=device.appId,
  File "/Users/andreas/code/x/venv-2.7/lib/python2.7/site-packages/u2flib_server/jsapi.py", line 68, in __getattr__
    (type(self).__name__, key))
AttributeError: 'DeviceRegistration' object has no attribute 'appId'
>>> start_authenticate('{}')
Traceback (most recent call last):
  File "<input>", line 1, in <module>
  File "/Users/andreas/code/x/venv-2.7/lib/python2.7/site-packages/u2flib_server/u2f_v2.py", line 228, in start_authenticate
    appId=device.appId,
  File "/Users/andreas/code/x/venv-2.7/lib/python2.7/site-packages/u2flib_server/jsapi.py", line 68, in __getattr__
    (type(self).__name__, key))
AttributeError: 'DeviceRegistration' object has no attribute 'appId'
>>> start_authenticate('banana')
Traceback (most recent call last):
  File "<input>", line 1, in <module>
  File "/Users/andreas/code/x/venv-2.7/lib/python2.7/site-packages/u2flib_server/u2f_v2.py", line 221, in start_authenticate
    device = DeviceRegistration.wrap(device)
  File "/Users/andreas/code/x/venv-2.7/lib/python2.7/site-packages/u2flib_server/jsapi.py", line 76, in wrap
    return data if isinstance(data, cls) else cls(data)
  File "/Users/andreas/code/x/venv-2.7/lib/python2.7/site-packages/u2flib_server/jsapi.py", line 57, in __init__
    self.update(json.loads(data.decode('utf-8')))
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/json/__init__.py", line 338, in loads
    return _default_decoder.decode(s)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/json/decoder.py", line 366, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/json/decoder.py", line 384, in raw_decode
    raise ValueError("No JSON object could be decoded")
ValueError: No JSON object could be decoded
>>>

When a signature check fails, cryptography.exceptions.InvalidSignature is raised. How are users supposed to know to catch an exception from the underlying cryptography library?

Given that this is the reference implementation of U2F for Python servers and this is security sensitive, exceptions and error conditions should be handled better and clearer. The example server can be crashed in a number of way by provided user input. The examples should be using best practices and be crash free.

My proposal: u2flib should define its own exceptions "U2FError" and could have different subclasses to classify input formatting errors "U2FMalformedInput" from signature errors "U2FSignatureError". Any exceptions that is raised from invoking u2flib that is not a "U2FError" should be treated as a bug.

[dainnilsson]

I agree with some of your points. I absolutely agree that raising an exception from the underlying library isn't very clean, so I've gotten rid of that.

However, I don't see an issue with re-using the built in exceptions in Python like ValueError, etc. From a security standpoint any uncaught exception should result in failure. Regardless if you raise a ValueError or a U2FError the case has to be handled, and the caller needs to be aware of it. For a dynamic language like Python I don't see much benefit to using custom exceptions in this case.

As you say, the example server isn't using best practices, and could be improved quite a bit.

[pelme]

I did not mean to imply that this in itself is a security problem, sorry for the confusion. Uncaught exceptions naturally leads to aborted authentication by itself. I was just arguing for having clear and straightforward error handling in u2flib to avoid mistakes when using the library.

I agree, both U2FError, ValueError and any Python exception must be handled and lead to authentication failure. However, they must be handled in different ways since they are different:

• When the user provides garbage or bad credentials I probably want to log the request and return a HTTP 400-ish error code. Currently a ValueError from the json library may happen in this case.

• u2flib may have bugs that we do not yet know about that also happens to throw ValueError. In that case, replying status code 400 to the client is not correct. We also want to log that and notify the developers and then maybe report the bug here. :)

The only way to use u2flib currently is basically to wrap all calls with except Exception: to be sure to handle every case. That hides bugs and is a python anti-pattern.

This is roughly how I would like to use u2flib:

try:
    try:
        start_authenticate(...)
    except U2FError:
        return HttpResponse(status=400, 'u2f authentication failed')
except Exception:  # This part is usually handled by Django, Flask etc
    logger.exception('u2f authentication crashed')

requests has a similar problem and solution (instead of u2f authentication they send http requests) that is solved with a base RequestException: http://docs.python-requests.org/en/master/_modules/requests/exceptions/

Thanks for the reply and working to improve the library. We use this library and deployed it for everyone in our company using a mixture of Yubikey NEOs, 4Cs and U2Fs and we are very happy with this setup!

[ziima]

+1 for @pelme 's request. It is quite common for Python libraries to provide custom exceptions and helps error handling of errors from those applications.

[tonydelanuez]

I've opened a pull request to start this work:

#49