Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#18943 closed defect (fixed)

sha3 testsuite fails on big endian hosts

Reported by: weasel Owned by: yawning
Priority: High Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: 0.2.8.2-alpha
Severity: Major Keywords: tor-core, crypto, TorCoreTeam201605
Cc: Actual Points:
Parent ID: Points: 0.4
Reviewer: Sponsor:

Description

Hi,

looking at the current https://buildd.debian.org/status/package.php?p=tor&suite=experimental, in particular at

https://buildd.debian.org/status/fetch.php?pkg=tor&arch=mips&ver=0.2.8.2-alpha-1&stamp=1461496211
https://buildd.debian.org/status/fetch.php?pkg=tor&arch=powerpc&ver=0.2.8.2-alpha-1&stamp=1461451662
https://buildd.debian.org/status/fetch.php?pkg=tor&arch=s390x&ver=0.2.8.2-alpha-1&stamp=1461436567

suggests that the sha3 test suite fails on big-endian hosts.

crypto/sha3: [forking] 
  FAIL ../src/test/test_crypto.c:506: assert(data == mem_op_hex_tmp): E241F247AF432EEDB73B6377C955F93F42B162AD7A2047E468381A59451EB178 vs 677035391CD3701293D385F037BA32796252BB7CE180B00B582DD9B20AAAD7F0
  [sha3 FAILED]
crypto/sha3_xof: [forking] 
  FAIL ../src/test/test_crypto.c:854: assert(out == mem_op_hex_tmp): 62FBE70B8BA99C650A17E57A921F558D0D24DF1C960463DE4DEAB998B4287550F3B909E83FA6B202873F3DAF9AE3A816014814386C8534CAF25E828F80AB0B8E233D669CCC0C0A94213896DB9018714D096C82AF8B52D68906B28F636CDE99EC5C78C31D992A66E4D957C41AFBD27A776AD3F486728BE7B4AE5DBA090AA6B3D0B858E7730CF59727E505F757E8441BAD63639FABFB40AE3F647DECDB7AF60BC6B4A93C2F3300A97698BC84B0AAEDD8FC278CC1AF9C33B09712443CD5F1D4713582EA1E40C0F3C4F2D3D854747803EC55EAD1ED512E964053A193137EB9BC4DD97D619F679401AF7B152D264793E7F886C0FECA5F5D4C9F2AB01C66558256BC2FE8E322F3B8D159960E53B63D7EF207588B402018F0C1AA8CEEDDB44BA32071F5D3EBB426CFEC939462BCE590BE7CD968534552103A38C428B36F8E832F05C7BDFEC0935C2893C0D875C0A43089A484CF4F60E2A80434BBB90068870ACADCE6B59521B19ECECDD14B237D0E149FE40ABC2188DA16C0527A19FE9584BF925199CAB95B9504561E24D648F64A088C58A336D78111852C4F8782882D9F7D9AF7B82960D682D83DD5FD681DF07BD47B116393DFEDEBDCA3DB464B68CA9DBF6AD3756FCFAE3BBD7E4B4737D8556081C855414A722EDEB7FF4B31AF4F3FF5F65368A9CE2F7BAA3507CFAFBF455F164143BC6F5CE4554F695EFAD65513116C26BA915750 vs 8A5199B4A7E133E264A86202720655894D48CFF344A928CF8347F48379CEF347DFC5BCFFAB99B27B1F89AA2735E23D30088FFA03B9EDB02B9635470AB9F1038985D55F9CA774572DD006470EA65145469609F9FA0831BF1FFD842DC24ACADE27BD9816E3B5BF2876CB112232A0EB4475F1DFF9F5C713D9FFD4CCB89AE5607FE35731DF06317949EEF646E9591CF3BE53ADD6B7DD2B6096E2B3FB06E662EC8B2D77422DAAD9463CD155204ACDBD38E319613F39F99B6DFB35CA9365160066DB19835888C2241FF9A731A4ACBB5663727AAC34A401247FBAA7499E7D5EE5B69D31025E63D04C35C798BCA1262D5673A9CF0930B5AD89BD485599DC184528DA4790F088EBD170B635D9581632D2FF90DB79665CED430089AF13C9F21F6D443A818064F17AEC9E9C5457001FA8DC6AFBADBE3138F388D89D0E6F22F66671255B210754ED63D81DCE75CE8F189B534E6D6B3539AA51E837C42DF9DF59C71E6171CD4902FE1BDC73FB1775B5C754A1ED4EA7F3105FC543EE0418DAD256F3F6118EA77114A16C15355B42877A1DB2A7DF0E155AE1D8670ABCEC3450F4E2EEC9838F895423EF63D261138BAAF5D9F104CB5A957AEA06C0B9B8C78B0D441796DC0350DDEABB78A33B6F1F9E68EDE3D1805C7B7E2CFD54E0FAD62F0D8CA67A775DC4546AF9096F2EDB221DB42843D65327861282DC946A0BA01A11863AB2D1DFD16E3973D4
  [sha3_xof FAILED]

Child Tickets

Change History (15)

comment:1 Changed 4 years ago by yawning

Keywords: tor-core crypto added
Owner: set to yawning
Priority: MediumHigh
Severity: NormalMajor
Status: newassigned

Yeah, the code is LE specific. The two xorin calls (one from the foldP macro and the other in finalize) are the only places that need changes. I'll deal with this after I wake up, though I do not have access to a non-LE target at the moment.

comment:2 Changed 4 years ago by yawning

Status: assignedneeds_review

This *should* fix Big Endian targets. Additionally while I'm here, the xorin call probably would have broken at a later date on ARM/PPC/etc when src isn't 8 byte aligned, so I addressed that as well (dst is fine assuming reasonable compiler behavior, the Keccak permutation code will also break if it's not properly aligned.).

https://git.schwanenlied.me/yawning/tor/src/bug18943

AFAIK the only code that calls this as of yet is the unit tests, and everyone else doing development that uses SHA3/SHAKE (the rng branch and the prop 224 people) haven't merged yet and use Intel systems anyway...

Edit: Point to the branch instead of the commit (since I force pushed since setout also should byteswap).

Last edited 4 years ago by yawning (previous) (diff)

comment:3 Changed 4 years ago by cypherpunks

According to C11 identifiers that begin with an underscore are reserved.

Also i would move line 242 to line 246 so the change becomes similar to that of foldP and setout and the comment wouldn't need to be duplicated.

comment:4 Changed 4 years ago by weasel

on 0c95e52c17da1e11f131d00ea9f52e60a70a41c4:

gcc -DHAVE_CONFIG_H -I. -I..  -I../src/ext -Isrc/ext -I../src/ext/trunnel -I../src/trunnel -I../src/common -Isrc/common -I../src/ext/trunnel -I../src/trunnel -I../src/or -Isrc/or -DSHARE_DATADIR="\"/usr/local/share\"" -DLOCALSTATEDIR="\"/usr/local/var\"" -DBINDIR="\"/usr/local/bin\"" -I../src/common     -g -O2 -D_FORTIFY_SOURCE=2 -fstack-protector-all -Wstack-protector -fwrapv --param ssp-buffer-size=1 -fPIE -fasynchronous-unwind-tables -Wall -fno-strict-aliasing -W -Wfloat-equal -Wundef -Wpointer-arith -Wstrict-prototypes -Wmissing-prototypes -Wwrite-strings -Wredundant-decls -Wchar-subscripts -Wcomment -Wformat=2 -Wwrite-strings -Wmissing-declarations -Wredundant-decls -Wnested-externs -Wbad-function-cast -Wswitch-enum -Werror -Winit-self -Wmissing-field-initializers -Wold-style-definition -Waddress -Wmissing-noreturn -Wstrict-overflow=1 -Wnormalized=id -Woverride-init -Wextra -Warray-bounds -Wlogical-op -MT src/ext/keccak-tiny/src_ext_keccak_tiny_libkeccak_tiny_a-keccak-tiny-unrolled.o -MD -MP -MF src/ext/keccak-tiny/.deps/src_ext_keccak_tiny_libkeccak_tiny_a-keccak-tiny-unrolled.Tpo -c -o src/ext/keccak-tiny/src_ext_keccak_tiny_libkeccak_tiny_a-keccak-tiny-unrolled.o `test -f 'src/ext/keccak-tiny/keccak-tiny-unrolled.c' || echo '../'`src/ext/keccak-tiny/keccak-tiny-unrolled.c
../src/ext/keccak-tiny/keccak-tiny-unrolled.c: In function 'hash':
../src/ext/keccak-tiny/keccak-tiny-unrolled.c:361:8: error: 's.delim' may be used uninitialized in this function [-Werror=maybe-uninitialized]
   if (s->delim != KECCAK_DELIM_XOF)
        ^
../src/ext/keccak-tiny/keccak-tiny-unrolled.c:402:16: note: 's' was declared here
   keccak_state s;
                ^
cc1: all warnings being treated as errors
Makefile:4433: recipe for target 'src/ext/keccak-tiny/src_ext_keccak_tiny_libkeccak_tiny_a-keccak-tiny-unrolled.o' failed

comment:5 Changed 4 years ago by weasel

and

crypto/digests: OK
crypto/sha3: [forking]
  FAIL ../src/test/test_crypto.c:506: assert(data == mem_op_hex_tmp): 8456C1F3F441B8B700A83A83788821397AC2EE1FFF3905DE2F1B8A00718582E9 vs 677035391CD3701293D385F037BA32796252BB7CE180B00B582DD9B20AAAD7F0
  [sha3 FAILED]
crypto/sha3_xof: [forking]
  FAIL ../src/test/test_crypto.c:854: assert(out == mem_op_hex_tmp): 4BCC68A9AB88A66E1ECB067D07E6ABB2E77BA5A9AB9C05FA9EE7242B27A2C09AAE83D4CBDA6F1FBAB2B9998A0A26C8C124174DC174633E5C0DC538739A6424C80A9611A5587D9AB93DE6551AD2B75014F6099FF8F00EB939A85D344B47D707BD417DF779B7FB0E9D755EBA29812FC6387461251C923FFF677271064DFD952196F33CAE28CB1C01E7CE62129A1A15DD2331BAE6E97B991F85E357D4A335BD3900AEA8D69015CD9F0DF93529532B1C457DD32EA1F269A9E019AB88582928B91ACD2BB0F58974C183108642050F21D66111D0CB426BB995501D2DD4480539A0ABA2D04E2ECCB231DEADA65EB9E7BBBB37152D51ACE61D4D06D3507864399FDFE7C37E47D47B2760126163FB50D653284623F4DCD628BB1DBA2587BA0C6D4DB3E0800961CF2D002ECF98E5E8CEEA6E3DD2DCE2082FE2B677BAE0CAD28AE379BF7DE010739CAF199F9A4EE19CC94AA2AB6F1A0BB9451A57AA947B71073B3372A522983424B78CE6DA945FC572EBE3A7A89689B51F9BB2717D62C1EF16E7BE4062480DE6C66C5216462A0AFF27A5A84E6DFA9217F7140E88D609CFA77F0420A1919BE7A5D825288B69A35227FF07C77E08E7069DD3BDC86DA8773C6709719FEA682ED1EC9471180E57C58AB2F0EB0463792BB9DD8C79EB96446C0193E84788A454DF0CA66EDE8BC0E89E0334C0041C8DBCE1C22D3DA77E467EA187484CBBFE228191F5 vs 8A5199B4A7E133E264A86202720655894D48CFF344A928CF8347F48379CEF347DFC5BCFFAB99B27B1F89AA2735E23D30088FFA03B9EDB02B9635470AB9F1038985D55F9CA774572DD006470EA65145469609F9FA0831BF1FFD842DC24ACADE27BD9816E3B5BF2876CB112232A0EB4475F1DFF9F5C713D9FFD4CCB89AE5607FE35731DF06317949EEF646E9591CF3BE53ADD6B7DD2B6096E2B3FB06E662EC8B2D77422DAAD9463CD155204ACDBD38E319613F39F99B6DFB35CA9365160066DB19835888C2241FF9A731A4ACBB5663727AAC34A401247FBAA7499E7D5EE5B69D31025E63D04C35C798BCA1262D5673A9CF0930B5AD89BD485599DC184528DA4790F088EBD170B635D9581632D2FF90DB79665CED430089AF13C9F21F6D443A818064F17AEC9E9C5457001FA8DC6AFBADBE3138F388D89D0E6F22F66671255B210754ED63D81DCE75CE8F189B534E6D6B3539AA51E837C42DF9DF59C71E6171CD4902FE1BDC73FB1775B5C754A1ED4EA7F3105FC543EE0418DAD256F3F6118EA77114A16C15355B42877A1DB2A7DF0E155AE1D8670ABCEC3450F4E2EEC9838F895423EF63D261138BAAF5D9F104CB5A957AEA06C0B9B8C78B0D441796DC0350DDEABB78A33B6F1F9E68EDE3D1805C7B7E2CFD54E0FAD62F0D8CA67A775DC4546AF9096F2EDB221DB42843D65327861282DC946A0BA01A11863AB2D1DFD16E3973D4
  [sha3_xof FAILED]

comment:6 in reply to:  3 Changed 4 years ago by yawning

Status: needs_reviewneeds_revision

Replying to cypherpunks:

According to C11 identifiers that begin with an underscore are reserved.

Don't care, it's macros swiped from src/ext/csiphash.c. Abstracting it out into a common header file would be a good idea in the future, but is beyond the scope of this ticket.

Also i would move line 242 to line 246 so the change becomes similar to that of foldP and setout and the comment wouldn't need to be duplicated.

Meh.

comment:7 in reply to:  5 Changed 4 years ago by yawning

Addressed the warning in a fixup. I'll deal with the other part of this later with a qemu ppc target.

comment:8 Changed 4 years ago by nickm

Keywords: TorCoreTeam201605 added

Calling all non-needs_information tickets for May.

comment:9 Changed 4 years ago by nickm

Points: small-remaining

comment:10 Changed 4 years ago by yawning

Status: needs_revisionneeds_review

https://git.schwanenlied.me/yawning/tor/src/bug18943_squashed

Passes the sha3 tests on debian stable running in a ppc qemu VM, and even fixes cpunk's nitpicks which will probably be replaced with other nitpicks.

comment:11 in reply to:  10 Changed 4 years ago by cypherpunks

Replying to yawning:

https://git.schwanenlied.me/yawning/tor/src/bug18943_squashed

Passes the sha3 tests on debian stable running in a ppc qemu VM, and even fixes cpunk's nitpicks which will probably be replaced with other nitpicks.

Thanks for fixing the nitpicks and for making IMO a beautiful patch.

You're right, there is one more nitpick (sorry). The i variable declarations in the endianness conversion helpers can be placed inside the for loops (like is done in the xorin8 and setout8 functions). This will make the helpers even more compact and make the for loops consistent with the rest of the changes.

comment:12 Changed 4 years ago by nickm

Hi, Yawning! This looks fine to me; there are optimizations we might make, but they can wait till we discover whether this is critical-path.

Also given that this is causing failing tests in 0.2.8, I'm considering cherry-picking this patch to 0.2.8. Please let me know if you think that's a poor idea. (It should be harmless.)

comment:13 in reply to:  12 Changed 4 years ago by yawning

Replying to nickm:

Hi, Yawning! This looks fine to me; there are optimizations we might make, but they can wait till we discover whether this is critical-path.

Yeah, I thought about optimizing for the cases where the host is little endian, and where the host allows unaligned qword access but decided to get something that will basically always work, and this makes it obvious where byte swapping is needed.

Also given that this is causing failing tests in 0.2.8, I'm considering cherry-picking this patch to 0.2.8. Please let me know if you think that's a poor idea. (It should be harmless.)

Go for it.

comment:14 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

cherry-picked and merged forward! Thanks.

comment:15 Changed 4 years ago by isabela

Points: small-remaining0.4
Note: See TracTickets for help on using tickets.