Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[update] fix (unexploitable) BB'06 vulnerability in rsa_verify #8142

Merged
merged 1 commit into from
Jan 21, 2016

Conversation

FiloSottile
Copy link
Collaborator

The rsa_verify code was vulnerable to a BB'06 attack, allowing to forge
signatures for arbitrary messages if and only if the public key exponent is
3. Since the updates key is hardcoded to 65537, there is no risk for
youtube-dl, but I don't want vulnerable code in the wild.

The new function adopts a way safer approach of encoding-and-comparing to
replace the dangerous parsing code.

@FiloSottile
Copy link
Collaborator Author

More details: https://blog.filippo.io/bleichenbacher-06-signature-forgery-in-python-rsa/

@FiloSottile
Copy link
Collaborator Author

======================================================================
ERROR: test_rsa_verify (test.test_update.TestUpdate)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/rg3/youtube-dl/test/test_update.py", line 27, in test_rsa_verify
    signature, UPDATES_RSA_KEY))
  File "/home/travis/build/rg3/youtube-dl/youtube_dl/update.py", line 21, in rsa_verify
    signature = b'%x' % pow(int(signature, 16), key[1], key[0])
TypeError: unsupported operand type(s) for %: 'bytes' and 'int'

@yan12125
Copy link
Collaborator

yan12125 commented Jan 5, 2016

Well this format is unavailable in Python < 3.5. [1]

[1] https://www.python.org/dev/peps/pep-0460/

@jaimeMF
Copy link
Collaborator

jaimeMF commented Jan 5, 2016

TypeError: unsupported operand type(s) for %: 'bytes' and 'int'

With this alternate form the test passes in all python versions:

signature = ('%x' % pow(int(signature, 16), key[1], key[0])).encode()

Please run flake8 and fix the style issues.

@FiloSottile
Copy link
Collaborator Author

test_rsa_verify (test.test_update.TestUpdate) ... ok
test_rsa_verify (test.test_update.TestUpdate) ... ok
test_rsa_verify (test.test_update.TestUpdate) ... ok
test_rsa_verify (test.test_update.TestUpdate) ... ok
test_rsa_verify (test.test_update.TestUpdate) ... ok
test_rsa_verify (test.test_update.TestUpdate) ... ok

@jaimeMF
Copy link
Collaborator

jaimeMF commented Jan 13, 2016

Is there some reason to use the last versions.json? It uses a lot of space (for example the tar.gz file produced with python setup.py sdist goes from 784K to 868K), we could use a smaller file (like https://github.com/rg3/youtube-dl/blob/77b66b65bd7ad85d84a66b7cfc1944d7e03328cb/update/versions.json).

Additionally: due to my lack of knowledge I can't comment on the actual change, since you know more feel free to merge it after some time if nobody disagrees.

@FiloSottile
Copy link
Collaborator Author

Very good point. [Done]

The crypto itself was reviewed by a couple people that know what they are doing, so should be alright. Or at least better than the one before.

I just want to be sure I'm not bricking the update process.

@jaimeMF
Copy link
Collaborator

jaimeMF commented Jan 15, 2016

I just want to be sure I'm not bricking the update process.

Well, I have tried to update and it works. Feel free to merge when you think it's ready.

The rsa_verify code was vulnerable to a BB'06 attack, allowing to forge
signatures for arbitrary messages if and only if the public key exponent is
3.  Since the updates key is hardcoded to 65537, there is no risk for
youtube-dl, but I don't want vulnerable code in the wild.

The new function adopts a way safer approach of encoding-and-comparing to
replace the dangerous parsing code.
FiloSottile added a commit that referenced this pull request Jan 21, 2016
[update] fix (unexploitable) BB'06 vulnerability in rsa_verify
@FiloSottile FiloSottile merged commit 032f232 into ytdl-org:master Jan 21, 2016
@FiloSottile FiloSottile deleted the filippo/updates branch January 21, 2016 20:17
@yan12125
Copy link
Collaborator

yan12125 commented Mar 7, 2016

I've just learned hmac.compare_digest. Maybe this function can be used for comparison? It's available only in Python 2.7.7+ or 3.3+. Nevertheless, try to use it:

import hmac

# First try to use the safe comparison function to prevent timing analysis attack
if hasattr(hmac, 'compare_digest'):
    return hmac.compare_digest(expected, signature)
else:  # Otherwise, fallback to unsafe direct comparison
    return expected == signature

This funciton is written in C for providing timing-analysis proof. (https://hg.python.org/cpython/file/e0df94327586/Modules/_operator.c#l165) I guess it's impossible to port it to pure Python for older Python versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants