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

test_cjose_concatkdf_derive_* fail on big endian architectures (s390x, powerpc, ppc64, sparc64, ...) #70

Closed
moschlar opened this issue Apr 17, 2018 · 4 comments · Fixed by #71

Comments

@moschlar
Copy link

The tests test_cjose_concatkdf_derive_* fail on big endian architectures like s390x, powerpc, ppc64, sparc64, ...:

check_concatkdf.c:159:F:concatkdf:test_cjose_concatkdf_derive_simple:0: Assertion 'derived==expected' failed: keylen==%, derived==0x20, expected==0x566eb950
check_concatkdf.c:190:F:concatkdf:test_cjose_concatkdf_derive_ikm:0: Assertion 'derived==expected' failed: keylen==%, derived==0x20, expected==0x566ebb80
check_concatkdf.c:224:F:concatkdf:test_cjose_concatkdf_derive_moreinfo:0: Assertion 'derived==expected' failed: keylen==%, derived==0x20, expected==0x566ebd50

Maybe this is something similar to #38 where some bad casts were the source of the problem that go unnoticed on little endian, but make it fail on big endian.
However, I did not find something at a quick glance (except from the fact that &err gets used in both function calls there, is not nulled inbetween and is not checked - maybe that would help debugging).

cjose_concatkdf_create_otherinfo(alg, keylen, cjose_header_new(&err), &otherinfo, &otherinfoLen, &err);
derived = cjose_concatkdf_derive(keylen, ikm, ikmLen, otherinfo, otherinfoLen, &err);

More details and complete build logs can be found on the respective Debian Buildd page:
https://buildd.debian.org/status/package.php?p=cjose

There is also a Debian bug report tracking the issue:
https://bugs.debian.org/890907

@linuxwolf
Copy link
Member

@moschlar Re-reviewing the code with #38 in mind, the possible problem points I see are:

  • Applying a 32-bit value to a byte buffer (including some pointer arithmetic)
    static uint8_t *_apply_uint32(const uint32_t value, uint8_t *buffer)
  • More pointer arithmetic (via function calls)
    uint8_t *ptr = buffer;
  • More direct pointer arithmetic while appending successive digests
    uint8_t *ptr = buffer + offset;

As with last time, we don't have a big-endian environment to troubleshoot and test on, so any assistance with or access to such an environment would be valuable.

@moschlar
Copy link
Author

moschlar commented Apr 24, 2018

  • Applying a 32-bit value to a byte buffer (including some pointer arithmetic)

Ah yes, it seems to be called with value of anything but uint32_t:

I'll see if I can somehow access such an environment to test it (cc @christmart).

Could you maybe think of a simple way to fix these three occurences?

@linuxwolf
Copy link
Member

linuxwolf commented May 16, 2018

(finally returning from holiday ... )

Thanks @moschlar.

The protocol expects a uint32_t, but just about all the calls around it expect a size_t. I'll change the int to uint32_t (it is just a counter that can't be negative). For the others I'll make an explicit cast.

@moschlar
Copy link
Author

moschlar commented Aug 3, 2018

The Debian architecture builds are now working, too! :-)

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 a pull request may close this issue.

2 participants