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

Change KeyBasedEncryptor's padding #1508

Merged
merged 8 commits into from
Oct 5, 2021
Merged

Conversation

shreyamalviya
Copy link
Contributor

@shreyamalviya shreyamalviya commented Oct 4, 2021

What does this PR do?

Fixes #1490

PR Checklist

  • Have you added an explanation of what your changes do and why you'd like to include them?
  • Is the TravisCI build passing?
  • Was the CHANGELOG.md updated to reflect the changes?
  • Was the documentation framework updated to reflect the changes?

Testing Checklist

  • Added relevant unit tests?
  • Have you successfully tested your changes locally? Elaborate:

    Tested by running the Island and changing the configuration.

  • If applicable, add screenshots or log transcripts of the feature working

image

@codecov
Copy link

codecov bot commented Oct 4, 2021

Codecov Report

Merging #1508 (19dad89) into develop (af99482) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1508      +/-   ##
===========================================
- Coverage    42.99%   42.99%   -0.01%     
===========================================
  Files          477      477              
  Lines        14175    14174       -1     
===========================================
- Hits          6095     6094       -1     
  Misses        8080     8080              
Impacted Files Coverage Δ
...utils/encryption/encryptors/key_based_encryptor.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af99482...19dad89. Read the comment docs.

from monkey_island.cc.server_utils.encryption import KeyBasedEncryptor

PLAINTEXT = "password"
PLAINTEXT_UTF8 = "slaptažodis" # "password" in Lithuanian
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add some other characters like: љњќшжѓ or 的 必 弟 . Just to make sure.

Copy link
Contributor

@ilija-lazoroski ilija-lazoroski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Just take a look at the UT comment.

Comment on lines 41 to 45
def _pad(self, message: str) -> bytes:
return Padding.pad(message.encode(), self._BLOCK_SIZE)

def _unpad(self, message: str):
return message[0 : -ord(message[len(message) - 1])]
def _unpad(self, message: bytes) -> str:
return Padding.unpad(message, self._BLOCK_SIZE).decode()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these functions are just thin wrappers, I suggest getting rid of them and inlining the calls to Padding.

padded_plaintext = Padding.pad(...)
return base64.base64encode(cipher_iv + cipher.encrypt(padded_plaintext)).decode()

...

padded_plaintext = cipher.decrypt(...)
return Padding.unpad(padded_plaintext, self._block_size).decode()

def test_encrypt_decrypt_string_utf8_with_key():
encrypted = kb_encryptor.encrypt(PLAINTEXT_UTF8)
decrypted = kb_encryptor.decrypt(encrypted)
assert decrypted == PLAINTEXT_UTF8
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a test that encrypts plaintext that is exactly a multiple of _BLOCK_SIZE in length.

@mssalvatore
Copy link
Collaborator

This should get a changelog entry. We fixed a bug.

Copy link
Contributor

@ilija-lazoroski ilija-lazoroski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also have ж in Macedonian language. :)

Good job!

@shreyamalviya shreyamalviya merged commit 19765c7 into develop Oct 5, 2021
@shreyamalviya shreyamalviya deleted the encryptor-with-utf8-chars branch October 5, 2021 08:48
mssalvatore added a commit that referenced this pull request Oct 5, 2021
Catching this exception was a workaround for an issue that was resolved
in PR #1508.
mssalvatore added a commit that referenced this pull request Oct 12, 2021
Catching this exception was a workaround for an issue that was resolved
in PR #1508.
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.

Encryptor doesn't work with UTF-8 characters
4 participants