Warehouse: API Token + a bit flip at end of token wrongly allows upload

[Carreau]

Describe the bug
For fun I tried to swotch the last letter of my token and try to upload a new version on a test package.
The upload worked.

Expected behavior
Single bit flip in the token should not allow to upload.

To Reproduce

Here is my session. The token has been revoked so is in clear.

~/dev/undefined[master ✓] $ export FLIT_PASSWORD='pypi:AgEIcHlwaS5vcmcCJDliOWJhNmY0LWM2MTItNDVhZC1hOWNhLWI1MTZjODk3OTc5NAACOnsicGVybWlzc2lvbnMiOiB7InByb2plY3RzIjogWyJ1bmRlZmluZWQiXX0sICJ2ZXJzaW9uIjogMX0AAAYgRrROleEWvzqI1Z6v5KIxZ6gPjCzj_4vGl_3L6a4CJyM'
~/dev/undefined[master ✓] $ export FLIT_USERNAME='@token'
~/dev/undefined[master ✓] $ flit publish
Found 7 files tracked in git                                                                                              I-flit.sdist
Writing generated setup.py                                                                                                I-flit.sdist
Built sdist: dist/undefined-0.0.7.tar.gz                                                                                  I-flit.sdist
Copying package file(s) from /var/folders/93/61br0dvj6_1306cdc7t1c_5r0000gp/T/tmp980ap2jr/undefined-0.0.7/undefined       I-flit.wheel
Writing metadata files                                                                                                    I-flit.wheel
Writing the record of files                                                                                               I-flit.wheel
Built wheel: dist/undefined-0.0.7-py3-none-any.whl                                                                        I-flit.wheel
Using repository at https://upload.pypi.org/legacy/                                                                      I-flit.upload
Uploading dist/undefined-0.0.7-py3-none-any.whl...                                                                       I-flit.upload
Package is at https://pypi.org/project/undefined/                                                                        I-flit.upload
Using repository at https://upload.pypi.org/legacy/                                                                      I-flit.upload
Uploading dist/undefined-0.0.7.tar.gz...                                                                                 I-flit.upload
Package is at https://pypi.org/project/undefined/                                                                        I-flit.upload

Bump package version and change last token letter from M to N:

~/dev/undefined[master ✓] $ export FLIT_PASSWORD='pypi:AgEIcHlwaS5vcmcCJDliOWJhNmY0LWM2MTItNDVhZC1hOWNhLWI1MTZjODk3OTc5NAACOnsicGVybWlzc2lvbnMiOiB7InByb2plY3RzIjogWyJ1bmRlZmluZWQiXX0sICJ2ZXJzaW9uIjogMX0AAAYgRrROleEWvzqI1Z6v5KIxZ6gPjCzj_4vGl_3L6a4CJyN'
 ~/dev/undefined[master ✓] $ flit publish
Found 7 files tracked in git                                                                                              I-flit.sdist
Writing generated setup.py                                                                                                I-flit.sdist
Built sdist: dist/undefined-0.0.8.tar.gz                                                                                  I-flit.sdist
Copying package file(s) from /var/folders/93/61br0dvj6_1306cdc7t1c_5r0000gp/T/tmp8ihapmbx/undefined-0.0.8/undefined       I-flit.wheel
Writing metadata files                                                                                                    I-flit.wheel
Writing the record of files                                                                                               I-flit.wheel
Built wheel: dist/undefined-0.0.8-py3-none-any.whl                                                                        I-flit.wheel
Using repository at https://upload.pypi.org/legacy/                                                                      I-flit.upload
Uploading dist/undefined-0.0.8-py3-none-any.whl...                                                                       I-flit.upload
Package is at https://pypi.org/project/undefined/                                                                        I-flit.upload
Using repository at https://upload.pypi.org/legacy/                                                                      I-flit.upload
Uploading dist/undefined-0.0.8.tar.gz...                                                                                 I-flit.upload
Package is at https://pypi.org/project/undefined/

Wait what ?

Let's do other changes: switch ...yM to ...tM (incorrect) :

403 Client Error: The credential associated with user 'mbussonn' isn't allowed to upload to project 'undefined'. See https://pypi.org/help/#project-name for more information. for url: https://upload.pypi.org/legacy/

Ok, good.

back to ...yM

400 Client Error: File already exists. See https://pypi.org/help/#file-name-reuse for url: https://upload.pypi.org/legacy/

Make sens with correct credentials.

... change back M to N:

400 Client Error: File already exists. See https://pypi.org/help/#file-name-reuse for url: https://upload.pypi.org/legacy/

Well I shoudl get a 403, nto a 400.

Summary, changing the last letter of my token still make it a valid token to upload my project... that seem strange to me.

My Platform

OS X, using flit, most recent stable (1.3) – but that should not matter. My password removed from .pypirc.

[brainwane]

Thank you @Carreau for this bug report!

(I did some quick checking with another maintainer to verify that this isn't bad enough that we should deal with it privately. But for your and other people's reference in the future, if you believe you have discovered a security vulnerability in PyPI/Warehouse, please follow our security policy to notify us privately.)

A malfeasor would still have to get every other character of the token correct, so this isn't TOO awful, but I'd still like to get this fixed before we publicize the new beta feature much further (#5661). I'm open to being persuaded that I'm wrong!

[Carreau]

Yes, opened a bug report as this is only in beta and you asked for testers.

I agree it looks non critical, just want to make sure it does not hide
other issues. I'm tempted to think it comes from the base64 encoding. The
token is not padded with = (equal signs) at the end, so maybe the last few
bits don't matter ?

On Thu, Jul 25, 2019, 19:09 Sumana Harihareswara [e-mail]
wrote:

Thank you @Carreau https://github.com/Carreau for this bug report!

(I did some quick checking with another maintainer to verify that this
isn't bad enough that we should deal with it privately. But for your and
other peeople's reference in the future, if you believe you have discovered
a security vulnerability in PyPI/Warehouse, please follow our security
policy https://pypi.org/security/ to notify us privately.)

A malfeasor would still have to get every other character of the token
correct, so this isn't TOO awful, but I'd still like to get this fixed
before we publicize the new beta feature much further (#5661
https://github.com/pypa/warehouse/issues/5661). I'm open to being
persuaded that I'm wrong!

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/pypa/warehouse/issues/6265?email_source=notifications&email_token=AACR5T3KVVKF5MWDF2PNP33QBJMHNA5CNFSM4IG7XIKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD23I7XQ#issuecomment-515280862,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AACR5T2BN665SNUDDBNPNI3QBJMHNANCNFSM4IG7XIKA
.

[dstufft]

This is just completely down to base64 encoding nonsense.

>>> from base64 import urlsafe_b64decode as d
>>> good = d("AgEIcHlwaS5vcmcCJDliOWJhNmY0LWM2MTItNDVhZC1hOWNhLWI1MTZjODk3OTc5NAACOnsicGVybWlzc2lvbnMiOiB7
InByb2plY3RzIjogWyJ1bmRlZmluZWQiXX0sICJ2ZXJzaW9uIjogMX0AAAYgRrROleEWvzqI1Z6v5KIxZ6gPjCzj_4vGl_3L6a4CJyM=")
>>> bad = d("AgEIcHlwaS5vcmcCJDliOWJhNmY0LWM2MTItNDVhZC1hOWNhLWI1MTZjODk3OTc5NAACOnsicGVybWlzc2lvbnMiOiB7I
nByb2plY3RzIjogWyJ1bmRlZmluZWQiXX0sICJ2ZXJzaW9uIjogMX0AAAYgRrROleEWvzqI1Z6v5KIxZ6gPjCzj_4vGl_3L6a4CJyN=")
>>> good == bad
True

Those two strings base64 decode to the exact same bytes, so there's not a lot we can do except either use a less malleable encoding standard.

[Carreau]

Thanks for the info. Readong on it more it seem that at worse the last 4 bits are thrown away.

Would it make sens to amke sure the last "thrown away" bits are always 0 ? I'm not a security expert so not quite sure about the implications. I'm thinking for example at Bots that make sure no credentials are leaked on GH, or similar that would fail to match a different token..

[njsmith]

Maybe just reject any token where encode(decode(token)) != token?

[woodruffw]

Would it make sens to amke sure the last "thrown away" bits are always 0 ? I'm not a security expert so not quite sure about the implications.

I'm not sure we could do this, at least not without fiddling with the internals of the Macaroon serialization format.

I'm thinking for example at Bots that make sure no credentials are leaked on GH, or similar that would fail to match a different token..

As far as automated token scanning goes, I believe the current plan is to support GitHub's credential alerting system. Theirs is pattern based and (for PyPI) will probably be keyed on the pypi: token prefix, so discrepancies in trailing bits won't matter to it.

[brainwane]

Thanks for the discussion & clarification. Sounds like this isn't a flaw we need to address right now, so I'm taking it out of the scope of what we need to do in this milestone.