Description: in the file sal/rtl/digest.cxx in the endSHA function, there is a bug. The line: if (i >= (DIGEST_LBLOCK_SHA - 2)) should be if (i > (DIGEST_LBLOCK_SHA - 2)) This bug likely is seen in ALL of the endHASH() functions since they look very similar. What happens is if the last limb of the hash is filled with exactly 52 to 55 bytes of data, then an extra limb (64 bytes) is added into the computation. this makes the hashes for any string that is 52 >= mod(length) <= 55 Steps to Reproduce: 1. perform an SHA1 computation against a string 52 to 55 bytes long 2. do an sha1 with the Libre code AND of the string: 1012345678901234567890123456789012345678901234567890 3. the results (hex encoded) from the Libre code are 90a461ee9cc69cedaeb25c2dc5cc62544ebd5241 4. perform same thing using standard unix tool: echo -n 1012345678901234567890123456789012345678901234567890 | sha1sum 5. The results of step 4 are 9cb1dab34448c1ea460da1f8736869c8852f212f *- 6. the results are not the same. sha1sum gives correct results while Libre gives incorrect results. I have researched and have been able to 100% mimic the Libre code by adding 64 additional null bytes of padding (i.e. an extra limb to the hash computation) 7. I am almost certain all other hashes handled in the digest.cxx are similarly buggy, but I have not tested any of them. Actual Results: sha1(1012345678901234567890123456789012345678901234567890) 90a461ee9cc69cedaeb25c2dc5cc62544ebd5241 Expected Results: echo -n 1012345678901234567890123456789012345678901234567890 | sha1sum 9cb1dab34448c1ea460da1f8736869c8852f212f *- Reproducible: Always User Profile Reset: No Additional Info: NOTE, the hashes are used for password validation and possibly other things. Since there was buggy SHA1 which produced data stored in files, then correcting this buggy sha1 behavior may render these user files non usable! Beware! NOTE, I did not find this using LibreOffice. I work on another OSS project (john), and we found this issue from data files which did not meet SHA1 specification. User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
think we can confirm as there is a disabled test for a long sha1 which was failing which succeeds with your suggested change
Looking at the endMD5(), I am almost certain it has the same bug. the bug comes from incrementing the i variable to point past the end of the non-null data in the limb, and then comparing it to make sure there are still 8 bytes (to place the bit count into). But since 'i' was already incremented, then you can not compare to equal that offset. It is a pretty simple off by 1 bug, but hard to spot because just looking at the code, it 'appears' to be right. NOTE, I am not sure of all the ways SHA1 is used within the software, so 'fixing' this may not be trivial. Data which was saved using the buggy hashes will fail to checksum the same, IF this code is fixed. If all that the code is doing, is providing a stored checksum to later be used to make sure that a password is correct, or to check for things like the document being unmodified, then if you change this code, those checks will no longer succeed and that being on existing files / data. If that is the case, it is possible that this bug should be left intact. There is NO data security issues, it simply does not match the FIPS results.
... and that data will fail to be interoperable with other compliant suites. I suppose we should simply fix it and make a notice to use older versions in case of problematic data
Or keep both for import, trying old code in case correct one fails, but only write correct.
The legacy busted code on import sounds correct. As long as it is only used upon a hash failure and upon any write the correct code is used to update the file. but that is way beyond the scope of my knowledge of this project (which is zero knowledge). All I did was told about the problem, and investigated. I first looked at the code and got an idea about what 'might' be wrong, and was able to quickly convert a stock 'working' sha1 implementation to provide the same (wrong) results as the LibreOffice was returning. I then cut the code for sha1 out of digest.cxx, converted to C, put in a main that would sha1 a command line argument or contents of a file, and was then able to detect and make sure this bug only was happening when the length of the last sha1 limb was 52, 53, 54, 55. It was then very easy to simply find the change that corrected the results. Btw, discussion of the bug can be seen here: https://github.com/magnumripper/JohnTheRipper/issues/3089
Just for information, there's a patch from Caolán to review on gerrit: https://gerrit.libreoffice.org/#/c/47683/
To me, that patch looks good, and the test case is a 52 byte value, which was one of the lengths which would trigger this bug. NOTE, I am almost certain that the MD5 hash has the exact same bug. Both MD5 and SHA1 use the same Merkle–Damgård construction. This is where the data is appended with a set bit all bits to the end of a full buffer limb are cleared, and the count of bits is placed at the end of buffer into the last 8 bytes. So if the data fits into 55 or less bytes in the last buffer, that IS the last buffer. If the data in the last buffer is 56 or more bytes, then a new buffer use used, the set bit is appended to the data (if it can be, or it is the first byte of this extra buffer is the length of data was an even mod(64) length) any unused data bytes in the end that last data buffer are cleared then it is hashed. Then that last hash limb all bits are cleared (unless the 0x80 was the first byte), and the 8 byte integer of bit count of data is placed at the last 8 bytes of this limb, and it is hashed. The buggy code simply adds this extra limb if there are more than 51 bytes of data in the last limb to hash. It should be more than 55 bytes (thus the off by one). I am sure endMD5() has the exact same bug in it. I do not know your project, or know that you have any test cases. But any data where n=length(data)%64 is in the range of 52 to 55 bytes will also have the exact same bug in MD5 as was corrected in the SHA1 code. For MD5, this is the line of code that needs changed (in endMD5() method) ```diff i += 1; - if (i >= (DIGEST_LBLOCK_MD5 - 2)) + if (i > (DIGEST_LBLOCK_MD5 - 2)) { for (; i < DIGEST_LBLOCK_MD5; i++) ``` The other core hash in this source file (MD2) does not use the Merkle–Damgård construction, so is not impacted by the code author's original bug or lack of testing.
I am not able to write a review, but I too (as did Michael Stahl) would fail the code review. The code is fine, BUT since this function is used in the ODF encryption scheme, all files current users have saved that are encrypted would become unreadable. There probably needs to be another flag added to the CTX structure for sha1, listing if the hash should be correct or buggy. Then when importing the file and ODF needs decrypt, first try the passscode with proper SHA1. If the checksum fails, then re-try but set the flag for 'buggy' SHA1. If that succeeds, then process the file as normal. However on save, only use the 'proper' SHA1 functionality. This will fix the 'bug', make your files compliant, and not cause any problems for your existing customers.
Michael Stahl committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=162ffd57de49b1f007b88fb247a592acbac0aaf7 (related:tdf#114939) connectivity: use real SHA1 It will be available in 6.1.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Michael Stahl committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=f66fbd947f70f6be6b22ab372facaeb9e2fb63ae tdf#114939 filter: don't use StarOffice SHA1 in MS Office filters It will be available in 6.1.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Michael Stahl committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=0b7c3b7d9fa71f59eed75c3e80e5e12245c5e1c5 tdf#114939 officecfg,sfx2: always use AES/SHA256 in ODF 1.2 It will be available in 6.1.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Michael Stahl committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=9188ea83c346fdc2f668178ae7538665a1b09c02 tdf#114939 package,comphelper: Try both real SHA1 and StarOffice SHA1 It will be available in 6.1.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Michael Stahl committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=50382b9e9256d7361e3770daa654fb8d09448635 tdf#114939 package: change ODF 1.1 export to use real SHA1 It will be available in 6.1.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Michael Stahl committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=9ef1734f03a008545a01fd394dd0e979bb230a0f tdf#114939 sfx2: notify user of non-interoperable passwords It will be available in 6.1.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Michael Stahl committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=efc06e9bb696110350ab3e14344de53db992280e tdf#114939 sal: deprecate rtl_digest_*SHA* and rtl_digest_PBKDF2 It will be available in 6.1.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
This affects ODF in multiple ways: * With non-default settings ODF 1.1 or ODF "1.2 Extended (compatibility mode)", the ODF package encryption uses SHA1 for the start-key derivation of PBKDF2 and SHA1/1k for the checksums of the files. However, it's quite hard to actually create an affected document in practice: - default version is ODF 1.2 extended - the XML files have random comments of >= 903 uncompressed bytes added in "makeXMLChaff()" - the content.xml has > 1k (uncompressed) of namespace declarations - passwords of 52 bytes are somewhat hard to remember Probably you'd have to add a really tiny image or so to hit the bug. * In all ODF versions, ODF package encryption PBKDF2 is implemented with SHA1, however it is not affected because of the start-key derivation round that generates a 20-byte or 32-byte input for PBKDF2. * ODF "password-to-modify" extension: this is stored in settings.xml so there isn't any interop to speak of currently; it uses PBKDF2 *without* start-key derivation so is affected. * Various "password" attributes that are handled centrally and with 2 different bugs in SvPasswordHelper, which is just a train-wreck. MS Office filters are also affected, although i haven't investigated how exactly. PDF export calls an affected function but apparently doesn't use the resulting hashes, oddly enough. There is a "master password" feature, in Tools->Options->LibreOffice->Security->Persistently save passwords for web connections->Master Password: * executeMasterPasswordDialog() calls rtl_digest_PBKDF2() with a user-provided password, so it's affected - this is stored somewhere in the user profile ... best not to change this, there is no interop benefit Outline of the plan to fix this nonsense: * ODF import: try both the correct & the buggy SHA1, take first that succeeds * ODF export, "1.2 Extended (compatibility mode)": switch to use SHA256/AES instead of SHA1/Blowfish - releases that only read the latter are > 5 years old. * ODF export, 1.1: it doesn't really matter if we continue to use the broken SHA1 or the fixed one, since the bug is so hard to trigger * ODF export, PBKDF (for ODF 1.1 and for password-to-modify): simply prevent 52 <= password <= 55 and reject this in the UI (but it's still possible to create such passwords via the API) * MSO import/export: always use correct SHA1 * Deprecate rtl_digest_*SHA* * do not fix the bug in endSHA() so that the rtl_digest_*SHA* functions can be used to import broken documents * TODO what to do about SvPasswordHelper::GetHashPassword
Michael Stahl committed a patch related to this issue. It has been pushed to "libreoffice-6-0": http://cgit.freedesktop.org/libreoffice/core/commit/?id=47c97efb78a574ba080e953ed219515ea71c2569&h=libreoffice-6-0 tdf#114939 officecfg,sfx2: always use AES/SHA256 in ODF 1.2 It will be available in 6.0.1. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Michael Stahl committed a patch related to this issue. It has been pushed to "libreoffice-6-0-0": http://cgit.freedesktop.org/libreoffice/core/commit/?id=936ead3c1df3f219e31419d37f9e1f7c8ed22663&h=libreoffice-6-0-0 tdf#114939 officecfg,sfx2: always use AES/SHA256 in ODF 1.2 It will be available in 6.0.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Michael Stahl committed a patch related to this issue. It has been pushed to "libreoffice-6-0": http://cgit.freedesktop.org/libreoffice/core/commit/?id=3761e01fd16a06468009c0de1b84026b2be1dda6&h=libreoffice-6-0 tdf#114939 package,comphelper: Try both real SHA1 and StarOffice SHA1 It will be available in 6.0.1. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Michael Stahl committed a patch related to this issue. It has been pushed to "libreoffice-6-0": http://cgit.freedesktop.org/libreoffice/core/commit/?id=0fbc3a1a90a649bc6353aca9fe3332556961f1a5&h=libreoffice-6-0 tdf#114939 package: change ODF 1.1 export to use real SHA1 It will be available in 6.0.1. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Michael Stahl committed a patch related to this issue. It has been pushed to "libreoffice-6-0": http://cgit.freedesktop.org/libreoffice/core/commit/?id=47b21d7bc342102c79b40a868709814ee771e49c&h=libreoffice-6-0 tdf#114939 sal: deprecate rtl_digest_*SHA* and rtl_digest_PBKDF2 It will be available in 6.0.1. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Michael Stahl committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=1929f23143e37cd50fc90425f5bd610827bff745 tdf#114939 vcl: don't use StarOffice MD5 in PDF export It will be available in 6.1.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Michael Stahl committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=ea794efe656d3ab2dd4e414aa023fd2983088e20 tdf#114939 sdext: don't use StarOffice MD5 in PDF import It will be available in 6.1.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Michael Stahl committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=18b022cadfa590df9dbefe0433b58838bcc3d2af tdf#114939 sal: fix endMD5() off-by-one It will be available in 6.1.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
as for MD5, which indeed has the same bug: * PDF export: lots of MD5 usage, this must use proper MD5 * PDF import: lots of MD5 usage, this must use proper MD5 * MS filters: Very odd - this uses rtl_digest_rawMD5(), which does *not* call endMD5(); no idea how to do the same with NSS API. (so apparently the competition couldn't get this right either?) * desktop, officeipcthread.cxx and dp_misc.cxx and start.c: This is an interesting case, URL of user profile is hashed with MD5 so that a newly started office.bin can contact an existing soffice.bin; if the user run multiple different LO versions that use the same user profile then they won't find each other if the URL is of the affected length... but probably very few users do that. * rtl_createNamedUuid(): this function is unused anyway, no idea if proper MD5 required * OpenGLHelper: some sort of caching going on, doesn't really matter * sc OpenCL: also for caching so because of the MS filters, i've decided it's best to fix the rtl_digest implementation; there appears to be no usage that requires bug-compatibility with this MD5 bug.
Michael Stahl committed a patch related to this issue. It has been pushed to "libreoffice-6-0": http://cgit.freedesktop.org/libreoffice/core/commit/?id=82e19b3a0cb0f51761fe2081729f7d739cae01eb&h=libreoffice-6-0 tdf#114939 filter: don't use StarOffice SHA1 in MS Office filters It will be available in 6.0.1. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Michael Stahl committed a patch related to this issue. It has been pushed to "libreoffice-6-0": http://cgit.freedesktop.org/libreoffice/core/commit/?id=17e01d692bf1ff13f3c2f5af599c688d52a911b6&h=libreoffice-6-0 tdf#114939 sal: fix endMD5() off-by-one It will be available in 6.0.1. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Michael Stahl committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=665ffc01e124029dc1412cc31e67878f697902ac tdf#114939 sal: document the bug in endSHA() It will be available in 6.1.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Stephan Bergmann committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=d27561ad071bbb2b7cabb45203515c2e1669476a tdf#114939: Verify rtl_digest_SHA1 computes broken results, by design It will be available in 6.1.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Michael Stahl committed a patch related to this issue. It has been pushed to "libreoffice-6-0-0": http://cgit.freedesktop.org/libreoffice/core/commit/?id=8f909506404b8a1d02152ebe78c44492abbbda6d&h=libreoffice-6-0-0 tdf#114939 package,comphelper: Try both real SHA1 and StarOffice SHA1 It will be available in 6.0.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Michael Stahl committed a patch related to this issue. It has been pushed to "libreoffice-6-0-0": http://cgit.freedesktop.org/libreoffice/core/commit/?id=2ba7890c6c07e1459e3a429c4641391ae5f06422&h=libreoffice-6-0-0 tdf#114939 sal: deprecate rtl_digest_*SHA* and rtl_digest_PBKDF2 It will be available in 6.0.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Michael Stahl committed a patch related to this issue. It has been pushed to "libreoffice-6-0-0": http://cgit.freedesktop.org/libreoffice/core/commit/?id=2fe6bfe0fe270a8cc45c52b95e94b87ce672a4a7&h=libreoffice-6-0-0 tdf#114939 filter: don't use StarOffice SHA1 in MS Office filters It will be available in 6.0.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Michael Stahl committed a patch related to this issue. It has been pushed to "libreoffice-6-0-0": http://cgit.freedesktop.org/libreoffice/core/commit/?id=36b39bb10da8d1887ee1bab755a71180909a84cf&h=libreoffice-6-0-0 tdf#114939 package: change ODF 1.1 export to use real SHA1 It will be available in 6.0.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Michael Stahl committed a patch related to this issue. It has been pushed to "libreoffice-6-0-0": http://cgit.freedesktop.org/libreoffice/core/commit/?id=977ef4449aa82e9b542fae402a32f1faeb3e619f&h=libreoffice-6-0-0 tdf#114939 sal: fix endMD5() off-by-one It will be available in 6.0.0. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Michael Stahl committed a patch related to this issue. It has been pushed to "libreoffice-5-4": http://cgit.freedesktop.org/libreoffice/core/commit/?id=32623337ba186bd7aac028920a1e1a6484e5e30f&h=libreoffice-5-4 tdf#114939 sal: fix endMD5() off-by-one It will be available in 5.4.5. The patch should be included in the daily builds available at http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: http://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
the outstanding issue about the SvPasswordHelper::GetHashPassword usage of the incorrect SHA1 (and its other more serious defects) is tracked in bug 115483. calling this fixed for now.
I have posted this same bug to the Apache Open Office groups bug server. That thread has received this message: https://bz.apache.org/ooo/show_bug.cgi?id=127661 >>The bug has been reported to Libre Office, (same source base), and is being >>actively handled there. >Well, maybe you could ask to the developer to publish his patch under AL v.2. >We'll happy to commit it into AOO trunk. I am not 100% sure what they are asking here, so I am simply posting the information to this bug report. At this time, I do not believe that AOO has taken any steps to correct this, such as the steps taken by the LibreOffice group. Jim.