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

Fix encoding of plaintext length indicator for secrets (fixes #2473) #2530

Closed
wants to merge 2 commits into from

Conversation

mwheckmann
Copy link

@mwheckmann mwheckmann commented Oct 19, 2018

Fixes: #2473

The encoding of the secret length indicator has likely been broken
since commit b21833f which introduced
Py3 support for secrets: the indicator was switched to using UTF-8 byte
strings and this caused the length indicator to consume 3 bytes instead
of just 2 under certain circumstances. For example when the plaintext
secret length is between 128 and 256 bytes long. This is because contrary
to latin-1 encoding, UTF-8 byte sting encoding consumes 2 bytes for
code points > 80. See the table at https://en.wikipedia.org/wiki/UTF-8#Description

The fix is to explicitely use 'latin-1' encoding for the length indicator.
This makes the code behave exactly as it did with the original Python2
implemenation while remaining compatible w/ Py3.

This fix does not alter the unpad/decode code path, only the code path which
encrypts new secrets. In other words, secrets that were stored in a broken state
will remain broken and what worked before will continue to work.

A test case which uses a 171 byte long plaintext string has also been added.
This test triggers the bug when the fix is not present.

…community#2473)

The encoding of the secret length indicator has likely been broken
since commit b21833f which introduced
Py3 support for secrets: the indicator was switched to using UTF-8 byte
strings and this caused the length indicator to consume 3 bytes instead
of just 2 under certain circumstances. For example when the plaintext
secret length is between 128 and 256 bytes long. This is because contrary
to latin-1 encoding, UTF-8 byte sting encoding consumes 2 bytes for
code points > 80. See the table at https://en.wikipedia.org/wiki/UTF-8#Description

The fix is to explicitely use 'latin-1' encoding for the length indicator.
This makes the code behave exactly as it did with the original Python2
implemenation while remaining compatible w/ Py3.

A test case which uses a 171 byte long plaintext string has also been added.
This test triggers the bug when the fix is not present.

This fixes netbox-community#2473
@mwheckmann
Copy link
Author

ok, Just noticed that this fails the Py2.7 tests. I wasn't aware that 2.7 was still supported. I'll try to fix this with subsequent commits.

@mwheckmann
Copy link
Author

Note that this code path fails under 2.7 even before my change if the plaintext length is between 128 and 256 chars: https://github.com/digitalocean/netbox/blob/0bb5d229e83def3d2c1c571f1cc5baaabc76c7e0/netbox/secrets/models.py#L404

We can see this w/ a simulation of the code path in running 2.7 :

$ python2.7
Python 2.7.15 (default, Sep 21 2018, 23:26:48) 
[GCC 8.1.1 20180712 (Red Hat 8.1.1-5)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> chr(170).encode()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
UnicodeDecodeError: 'ascii' codec can't decode byte 0xaa in position 0: ordinal not in range(128)

@cristian-ciobanu
Copy link

cristian-ciobanu commented Oct 19, 2018

In the next version of Netbox 2.5, Python 2 support will be dropped. Check this issue #2000

…y2 or py3

for the plaintext length indicator field.

Py2.7 cannot encode code points > 128 so fall back to chr(code_point)
without the "encode()" attribute when Python 2.x is detected.

This does not change the existing decrypt/unpad code paths. So whatever
is currently broken in the DB due to bug netbox-community#2473 will remain broken.
@mwheckmann
Copy link
Author

Fixed by using a separate code path for Py 2.7 or Py 3.x. I also tested creating secrets while running 2.7 and then upgrading to 3.6 using the same DB. The secrets created in 2.7 were still usable in 3.6.

Thanks for letting me know about #2000 @Cioby23, but 2.5 stills seems a ways off, so I'd like to get this into 2.4 as the secrets feature is completely unusable for longer text sizes as it stands now.

@jeremystretch
Copy link
Member

I appreciate the effort, but after fiddling with this for a bit all we really need to do is remove .encode() from the pad integers under Python 2 and force bytes() on them under Python 3. We also don't need a separate test method: we can just use a longer string for the plaintext under test_01_encrypt_decrypt().

@lock lock bot locked as resolved and limited conversation to collaborators Jan 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Secret encryption fails with 128+ characters under Python 2
3 participants