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

ARIA/CTR mode self test failures #1235

Closed
noloader opened this issue Sep 29, 2023 · 2 comments
Closed

ARIA/CTR mode self test failures #1235

noloader opened this issue Sep 29, 2023 · 2 comments

Comments

@noloader
Copy link
Collaborator

noloader commented Sep 29, 2023

Rabbit, RabbitWithIV, HC128 and HC256 were producing incorrect results when inString == outString as reported in GH #1231. We added self tests to detect the failures, and cleared the failures.

The new tests also revealed ARIA/CTR mode also has problems. The failure can be duplicated with ./cryptest.exe tv aria.

This bug will track ARIA/CTR.

noloader added a commit that referenced this issue Sep 30, 2023
ARIA SIMD code existed to perform an XOR and the end of encryption and decryption. It was a lot of work to save for the final XOR.
Worse, the final XOR seemed to be causing problems as described in GH #1235. Once we unrolled the XOR and used them when building outBlock, the 1235 issue went away.
noloader added a commit that referenced this issue Sep 30, 2023
This is another mystery to me. I do not know why unrolling the XOR into building of outBlock fixes this problem.
@noloader
Copy link
Collaborator Author

noloader commented Sep 30, 2023

This was cleared in two steps. The 2nd step, which fixed the issue, is Commit dde8e9fa23bc. The dde8e9f commit removed this block, and folded the XOR into building outBlock.

-	if (xorBlock != NULLPTR)
-		for (unsigned int n=0; n<ARIA::BLOCKSIZE; ++n)
-			outBlock[n] ^= xorBlock[n];

The 1st commit, Commit 5250ab2bf2b0, removed the SIMD code since it was no longer needed due to the second commit.

I do not know why this fixed ARIA. The two codes should have been equivalent.

@noloader
Copy link
Collaborator Author

I do not know why this fixed ARIA. The two codes should have been equivalent.

ARIA had undefined behavior. Ugh!

The working area - m_w - was too small. It should have been a SecWordBlock with room for 28 word-sized elements. Instead it only had room for 17 elements. Arg!!!

Even more baffling, Valgrind never alerted to the problem.

Also see Commit d3d23002f607.

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

No branches or pull requests

1 participant