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

Last byte of plaintext should not be ignored. #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Dzitu
Copy link

@Dzitu Dzitu commented Jun 27, 2017

Messages passed are byte arrays. There is no hint the array is NULL terminated string. Especially that length is given explicitly. Code should not assume final character is terminator in that case. Due to that substraction of 1 breaks the message (one byte of plaintext is ignored).

…erminated string. Especially that length is given explicitly. Code should not assume final character is terminator in that case. Due to that substraction of 1 breaks the message (one byte of plaintext is ignored).
@spaniakos
Copy link
Owner

will review!
thanks

@spaniakos
Copy link
Owner

the -1 have to be replaced in more than one function.
will implement the changes , thanks for the hint.

@palto42
Copy link
Contributor

palto42 commented May 6, 2018

I checked the other functions in AES.cpp and don't think that there is any other function where the "-1" would need to be replaced. In my few this pull request can be accepted as is.
Only other potential change is the description of this function in AES.h which currently states "* @param p_size the size of the byte array ex sizeof(plaintext)". It might be useful to add a note that the sizeof() function may be incorrect in case of strings. For example byte plain[] = "0123456789" will have sizeof(plain) = 11 since it was created from a string, but strlen(plain) = 10 would be correct.

palto42 added a commit to palto42/AES that referenced this pull request May 7, 2018
Combined proposed fixes of pull requests spaniakos#15 and spaniakos#16 plus the required change of the function `AES::CheckPad(byte* in,int lsize)`.
The function `AES::calc_size_n_pad`could be simplified by combining spaniakos#15 and spaniakos#16 and also ` `AES::CheckPad` is simpler because there is always padding added.
The definition of `byte arr_pad` in `AES.h` must also be aligned for the extra padding value 0x10.
@spaniakos
Copy link
Owner

spaniakos commented May 7, 2018

i will test the code.
you are correct about string. the problem with the strings are that the \0 is calculated as a character.
but since the library is supposed to handle bytes and not strings, it is better to have a function that will handle byte arrays and a separate function that will handle strings. the seconds function should have the -1 automatically when a string is given and should be a able to calculate the size on its own.

now for the padding, i saw that you you have made the padding array 0x01 to 0x10
that is also correct. as by using this kind of padding 0x10 = 16 the algorithm will be pkcs7 compliant. ( as it should have been from the beginning).

nevertheless, i will test your code against the tests and afterwards will accept the pull request.

@palto42
Copy link
Contributor

palto42 commented May 7, 2018

Hi @spaniakos
I've started to review the joint changes of #15 and #16 and plan to submit a pull in the next days. If you're interested, you can have a look at my branch "pad_fix" master...palto42:pad_fix (not tested yet).

@spaniakos
Copy link
Owner

will sure do!

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