maythamfahmi / CryptoNet

CryptoNet is simple, fast and a lightweight asymmetric and symmetric encryption library.
https://github.com/maythamfahmi/CryptoNet
MIT License
99 stars 19 forks source link

Encrypt and Decrypt byte[] without string conversion using AES #39

Closed ghost closed 1 year ago

ghost commented 1 year ago

When using the library to encrypt and decrypt PDF files received through an ASP.NET Core endpoint using AES, the PDF file after en- and decryption would be corrupted. I determined this was due to the byte[] being converted to a string before encryption and decryption.

Implementing the same methods without this conversion solved the issue. The existing EncryptContent and DecryptContent methods for strings were not modified.

maythamfahmi commented 1 year ago

Thanks for this, I have created bug #40 and will review your suggestion.

maythamfahmi commented 1 year ago

TK-SMF First, thank you for your PR and your suggestion.

Secondly, I have created bug issue #40 with your description.

Back to the code review, after investigating the issue, I agree with you, that when decrypting PDF, it makes the PDF corrupted, no doubt in that. the issue. The solution you have provided will work but still has issues. In one way it solves the PDF corruption issue but it introduces another issue. When I compare the original PDF before encryption and PDF after decryption, some bytes are missing. After spending some time and unit testing,

The problem really Encoding.ASCII in CryptoNetUtils.cs and the possible solution is in this answer https://stackoverflow.com/a/49106300/3088349

Here is my suggestion if you only change to CryptoNetUtils.cs

public static string BytesToString(byte[] bytes)
{
    return Convert.ToBase64String(bytes);
}

public static byte[] StringToBytes(string content)
{
    return Convert.FromBase64String(content);
}

And testing it, let me know I will approve it. Otherwise, let me know I am going to make a bugfix ASAP. But first need to validate the test results.

maythamfahmi commented 1 year ago

TK-SMF Wait before changing anything, I am still spending some time to figure out if that is the real solution. Because what I wrote might have some side effects that I am trying to figure out.

maythamfahmi commented 1 year ago

@TK-SMF now it is fixed on branch https://github.com/maythamfahmi/CryptoNet/tree/feature/41-major-imporvement

If you don't mind, pull the branch and test it for your solution, let me know so I can create a new PR and release an update Nuget package.

maythamfahmi commented 1 year ago

@TK-SMF I will close this PR and let's communicate on #41