Skip to content

Conversation

@denovosoftware
Copy link
Contributor

Incorrect auth tag is generated by DecodeStream method due to buffer reuse.
Also addresses issue with auth tags for empty data streams.
Solution is to update logic similar to logic in DecodeBytes methods.

…ased cipher produces incorrect tag when reusing buffers. Solution is duplicating logic in existing DecodeBytes method.
@MHumm
Copy link
Owner

MHumm commented Nov 9, 2022

Thanks for providing this pull request. I try to find the time to review it in the near future!

@MHumm
Copy link
Owner

MHumm commented Nov 19, 2022

I had a look at your commits now. While most things look clear enough to me theres one issue: there is no TestTDECGCM.TestDecodeStream method in Unit Tests/Tests/TestDECCipherModesGCM.pas, at least not in the current development or master branch.

There is no // FIXME comment at all in the whole project group at the moment.
How to accept the first two commits but leaving out the other one? Or what would happen if I accept a pull request which wants to modify source which is not there?

Ah, sorry. I'm not too accustomed with pull requests yet and overlooked the commit adding the TestDecodeStream method.

@MHumm MHumm merged commit 1b786eb into MHumm:master Nov 19, 2022
@MHumm
Copy link
Owner

MHumm commented Nov 19, 2022

I merged your pull request but: your two new unit tests TestDecodeStream and TestEncodeStream do crash now with access violations and invalid pointer operations etc. It looks like the calls with a 0 byte Size and empty Source and Dest params in DecodeGCM(Source, Dest: TBytes; Size: Integer) are the problem.

Since this version is online noew in development and master branch: could you please have a look and try to provide a fix as soon as time permits?

@MHumm MHumm changed the title Fix auth tag issues when using GCM cipher with steam-based methods Fix auth tag issues when using GCM cipher with stream-based methods Nov 19, 2022
@denovosoftware denovosoftware deleted the Fix-GCM-Streams branch November 21, 2022 21:14
@denovosoftware
Copy link
Contributor Author

Hi, sorry about that. Pull request #49 should fix those AV issues.

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.

2 participants