Repository navigation
Authenticated tag is wrong on streams #87
Description
Activity
Lets fix the bug first before discussing further functionality improvements. Do you suppose to pass the DataToAuthenticate in EncodeStream only on the last EncodeGCM call? Would that fix this bug?
- Hi!I think so. There should not be a differnce between a stream and a full block right? Maybe I‘ll try to tackle the issue in conjunction with the other one… (if you like) I never tried to create a pull request but this could be a nice possibility :)Von meinem iPhone gesendetAm 22.04.2025 um 19:45 schrieb Markus ***@***.***>: Lets fix the bug first before discussing further functionality improvements. Do you suppose to pass the DataToAuthenticate in EncodeStream only on the last EncodeGCM call? Would that fix this bug?—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***> MHumm left a comment (MHumm/DelphiEncryptionCompendium#87) Lets fix the bug first before discussing further functionality improvements. Do you suppose to pass the DataToAuthenticate in EncodeStream only on the last EncodeGCM call? Would that fix this bug? —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>
Feel free to provide a suggested fix. I'd add you to notice.txt as contributor ;-)
If it shouldn't work as pull request for you we'll find an alternative way. Just let me know here if that's the case.I was not able to commit anything to development - however I attached the files in "Source" and one file in the Unit test folder that
should cover tests for bugs #86 and #87.
Let me know if that helps.This is for block wise encoding only - there has been no work on the decoding part too. In addition it lacks some comments...
You are not able to commit, because you do not yet have commit permissions to this repo. The other way most folks prefere would be to create a fork, linkt that with this repo and then you can commit to your fork and create a pull request from that one.
I'll try to have a look at your code within the next few days.
Btw. may I close #86 now as fixed?Yes... #86 looks fixed now :)
I have an update that does the decoding part as well. Unfortunately the previous encode/decode stream tests do not work any more... I have no idea yet why, maybe you can find something,
Yes - the thing is the original stream unit tests fail now... Maybe the tests are based on the "original" assumption on how the streaming should work... I don't know. Can you take a look at it?
We need to check which test data is being used. I'm not sure at the moment whether DEC has "official" test data, means from a standard body, for GCM. If it has/unses that the stream and non stream tests should provide the same results.
Can you write a small test application comparing the use of some of the test data from the already implemented "non stream" unit tests and feed this to both: non stream methods of DEC and stream methods and compare whether that delivers the same results? I'm quite busy at the moment and will thus not find the time during the next few weeks to look at it. The comparison should show if we really have a bug in the stream handling of this algorithm and you can also implement as 3rd variant your fixed stream variant. This should provide the same results than the non stream code.
The last zip file I provided actually implements the following test:
- Create a buffer with 32k of bytes
- Define a small header that is used for the DataToAuthenticate property
- One time the buffer is fed to "EncodeBytes" and the second time to "EncodeStream"
- The Auth tags are then compared
- In addition the stream is decoded and checked with the updated GCM method
This test seems to work but the older stream tests fail now :/
Looking at the streaming tests - especially in the GCM section - they partly do not make a lot of sense to me...
The internal Encode/decode stream functions utilize a chunking scheme of 8kB per Block. From my perspective these
were ment to be called only once right?
The actuall stream testing function run "chunking" themself by multiple calls on Encode/Decode Stream with a chunk size.
This creates the same problem in the GCM as the initial described problem - multiple calls to Encode/DecodeStream include
multiple calculation on the AuthenticatedData item (aka on each call). So my question now is:- Is the library meant to be used that way? Actually having multiple EncodeBytes/EncodeStream calls in one "session"
- If so should - in the GCM case - one start such a session via a separate calls? (Begin End...)
- The current implementation does not allow this nevertheless the tests are actually pointing in that direction....
Hm, I'll start to look at the code you provided, but it may take some time before I get any results. About the tests with the chunks: I'm not 100% firm in the use of streams and there is another bugreport: #52. Most likely I created some test cases using chunks to investigate this one. The aim is to find out if the stream based stuff provides the same result if the data is fed into the stream in small chunks.
Ah yes... Daniel Marshalls last comment seems to describe exactly the same problem! The gcm method allows only for one call to Encode or EncodeStream (which is ok I think). My bugreport describes also the problem within EncodeStream that - if the stream is
larger than the internal chunk size of 8k - the calculated tag is wrong if there is additional non encrypted authentication data added.So... If you want to have both things working one would need to add additional internal states and eventually even a BeginEncode, EndEncode routine if you want to allow multiple calls to Encode/EncodeStream....
Thanks for this additional insight!
Does this mean that the tag is only wrong, if I add additional data to be authenticated multiple times and that after having added more than 8k of "normal" data already?If multiple calls to Encode/DecodeStream should be allowed (not sure yet how beneficial this would be), we might put the "End" into a new overload for Done, which would need to have the output stream passed as parameter I guess, so it can work on that. Begin is not specifically needed, as stuff requiring that can be done in the first Encode/Decode call or the constructor unless, one wants to reuse the cipher instance. But I guess in that case a new call of Init with a new IV etc. is necessary anyway and thus such "setup" code could be inserted there.
From my experience I never encountered a problem that needed multiple calls to encode/decode stream anyway - e.g. if you have
a really large file the encode routine would do the chunking anyway. I'm also not sure if for more sophisticated streaming (e.g. video streaming) one would have a specialized streaming object that would block the reading/writing until new data arrives.
The only thing to handle here is the dynamic stream size...Here is more information about this issue (albeit in parts in German):
https://www.delphipraxis.net/217186-umfrage-architekturfrage-zur-dec.html#post1548654Actually I see also the same problem with the new CCM block chaining mode - it operates the same way.
Of course, as I still need to find the time to work out how it should be designed. Best would be that it stays compatible with the current implementation, but I guess we cannot avoid the need to call Done.
But maybe we can add something to destructor which detects if Done has been called and if not raises some exception. But only in debug mode or when run from the IDE or so. That would be a big hint to the developers using DEC ;-)
The routine
EncodeStreamcreates a different authentication tag as theEncodeBytesif the fieldDatatToAuthenticateis used.The problems seems to be that EncodeStream splits the buffer in 8kB blocks and calls
EncodeGCMmultipletimes. Here the
DataToAuthenticatefield is used on every call which means that it is mengled into thetag calculatation on each iteration.
EncodeBytes calls the EncodeGCM routine only once on the whole buffer. Check out the attached project (unfortunately I could not upload a dpr file only a txt file...)
FailStreamEncrypt.txt
On a different matter... would it be possible to add an additional parameter to the EncodeStream function that allows
to change the endianess of the stream? There are built in classes that allow to operate on TBytes but not on streams.
(Or... did I miss something?)
My background here is to create an encrypted firmeware file for a microcontroller which uses an ARM CPU