Age | Commit message (Collapse) | Author |
|
Change-Id: I1597a8e77199445bf377dbe54adc3134bb04fd51
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/125748
Tested-by: Jenkins
Reviewed-by: Mike Kaganski <mike.kaganski@collabora.com>
|
|
Signing OOXML with 3 or more times didn't work as other ids
("idPackageObject", "idOfficeObject", ...) were not uniqe. This
change makes those ids unique by appending the signature id. The
signature ID is now generated for OOXML too, while previously it
was a hardcoded string ("idPackageSignature").
The test for signing multiple OOXML was written before, but didn't
catch the issues because it didn't assert the status of the
document after loading it again. This is which is now fixed (and
also added changed for the ODF test case).
Change-Id: Ifa20ea17498b117a4c57f6eddf82f8e83bc640bc
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/124571
Tested-by: Jenkins
Reviewed-by: Tomaž Vajngerl <quikee@gmail.com>
|
|
Signing the document 3 or more times produces an invalid signature.
The cause of this is that xmlsec is confused because we have 3
signatures, which all have the same SignedProperties with the ID
"idSignedProperties", but it expect them to be unique.
This issue is fixed by making the ID unique with adding the ID of
the Signature to the SignedProperties ID, so this makes them unique
inside the same Signature.
Also UnsignedProperties have a unique ID usign the same approach,
but they aren't referenced - luckily.
Change-Id: I53c7249a82fc0623586548db9fa25bdc0e7c4101
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/124278
Tested-by: Jenkins
Reviewed-by: Tomaž Vajngerl <quikee@gmail.com>
|
|
It turns out that the 2 backends NSS and MS CryptoAPI generate different
string representations of the same Distinguished Name in at least one
corner case, when a value contains a quote " U+0022.
The CryptoAPI function to generate the strings is:
CertNameToStr(..., CERT_X500_NAME_STR | CERT_NAME_STR_REVERSE_FLAG, ...)
This is documented on MSDN:
https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-certnametostra#CERT_X500_NAME_STR
NSS appears to implement RFC 1485, at least that's what the internal
function is named after, or perhaps one of its several successor RFCs
(not clear currently if there's a relevant difference).
This is now causing trouble if a certificate with such a DN is used in a
signature, created on WNT but then verified on another platform, because
commit 5af5ea893bcb8a8eb472ac11133da10e5a604e66
introduced consistency checks that compare the DNs that occur as strings
in META-INF/documentsignatures.xml:
xmlsecurity/source/helper/xmlsignaturehelper.cxx:672: X509Data cannot be parsed
The reason is that in XSecController::setX509Data() the value read from
the X509IssuerSerial element (a string generated by CryptoAPI) doesn't
match the value generated by NSS from the certificate parsed from the
X509Certificate element, so these are erroneously interpreted as 2
distinct certificates.
Try to make the EqualDistinguishedNames() more flexible so that it can
try also a converted variant of the DN.
(libxmlsec's NSS backend also complains that it cannot parse the DN:
x509vfy.c:607: xmlSecNssX509NameRead() '' '' 12 'invalid data for 'char': actual=34 and expected comma ',''
but it manages to validate the signature despite this.)
Change-Id: I4f72900738d1f5313146bbda7320a8f44319ebc8
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/124287
Tested-by: Jenkins
Reviewed-by: Michael Stahl <michael.stahl@allotropia.de>
|
|
Change-Id: Ifd1068066c8711a1fa96ff4ede1fa0017a221617
|
|
Sure, it would be better to figure out why it fails.
Change-Id: I4c9e8aa0a9a3cd421de08ec9c9ea2dcb1a242ab1
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/123875
Tested-by: Tor Lillqvist <tml@collabora.com>
Reviewed-by: Tor Lillqvist <tml@collabora.com>
|
|
With Fedora's nss-3.71.0-1.fc34.x86_64 there is the problem that
8 tests including testODFGood in CppunitTest/xmlsecurity_signing
fail because the crypto policy disallows SHA1 for signatures.
Apparently this particular policy bit was added in NSS 3.59:
https://bugzilla.mozilla.org/show_bug.cgi?id=1670835
For signatures, maybe it's not a good idea to override system policy
for product builds, so do it locally in the tests, at least for now.
Another option would be to set NSS_HASH_ALG_SUPPORT environmental
variable to something like "+SHA-1".
If similar problems turn up for encrypted documents in the future,
that should be fixed in product builds too of course, as encrypted
documents must always be decryptable.
Change-Id: I4f634cf5da1707fb628e63cd0cdafebdf4fc903f
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/123678
Tested-by: Michael Stahl <michael.stahl@allotropia.de>
Reviewed-by: Michael Stahl <michael.stahl@allotropia.de>
|
|
Tests added in commit 40d70d427edddb589eda64fafc2e56536953d274 don't
actually run on WNT but that wasn't obvious because commit
149df1fec6472e30582162e17e04c75aee91d26a prevented running them in
Jenkins on master, they failed only in the libreoffice-7-1 backport.
xmlsecurity/qa/unit/signing/signing.cxx(631) : error : Assertion
Test name: testODFDoubleX509Certificate::TestBody
assertion failed
- Expression: (nActual == SignatureState::NOTVALIDATED || nActual == SignatureState::OK)
- 2
This is an oddity where NSS claims the signature in the document is
valid but CryptoAPI claims it is invalid; the hashes passed into the
validation functions are the same. Just allow BROKEN as an additional
result value on WNT.
xmlsecurity/qa/unit/signing/signing.cxx(550) : error : Assertion
Test name: testODFX509CertificateChain::TestBody
equality assertion failed
- Expected: 0
- Actual : 1
The problem here is that with NSS the tests use a custom NSS database
in test/signing-keys so we need to make these certificates available for
CryptoAPI too.
The following one-liner converts the NSS database to a PKCS#7 that can
be loaded by CrytpAPI:
> openssl crl2pkcs7 -nocrl -certfile <(certutil -d sql:test/signing-keys -L | awk '/^[^ ].*,[^ ]*,/ { printf "%s", $1; for (i = 2; i < NF; i++) { printf " %s", $i; } printf "\n"; }' | while read name; do certutil -L -d sql:test/signing-keys -a -n "${name}" ; done) > test/signing-keys/test.p7b
Then one might naively assume that something like this would allow these
certificates to be added temporarily as trusted CAs:
+ HCERTSTORE hRoot = CertOpenSystemStoreW( 0, L"Root" ) ;
+ HCERTSTORE const hExtra = CertOpenStore(
+ CERT_STORE_PROV_FILENAME_A,
+ PKCS_7_ASN_ENCODING | X509_ASN_ENCODING,
+ NULL,
+ CERT_STORE_OPEN_EXISTING_FLAG | CERT_STORE_READONLY_FLAG,
+ path);
+ if (hExtra != NULL && hRoot != NULL)
+ {
+ BOOL ret = CertAddStoreToCollection(
+ hRoot,
+ hExtra,
+ CERT_PHYSICAL_STORE_ADD_ENABLE_FLAG,
+ 0);
+ SAL_DEBUG("XXX hExtra done " << ret);
+ }
There is no error from this, but it doesn't work.
Instead, check if CertGetCertificateChain() sets the
CERT_TRUST_IS_UNTRUSTED_ROOT flag and then look up the certificate
manually in the extra PKCS#7 store.
Change-Id: Ic9865e0b5783211c2128ce0327c4583b7784ff62
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/123667
Tested-by: Jenkins
Reviewed-by: Miklos Vajna <vmiklos@collabora.com>
|
|
Change-Id: I749e19f786ad006dffcd65dd1ee60e57c428f57b
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/123717
Tested-by: Jenkins
Reviewed-by: Julien Nabet <serval2412@yahoo.fr>
|
|
... to avoid hidden cost of multiple COW checks, because they
call getArray() internally.
This obsoletes [loplugin:sequenceloop].
Also rename toNonConstRange to asNonConstRange, to reflect that
the result is a view of the sequence, not an independent object.
TODO: also drop non-const operator[], but introduce operator[]
in SequenceRange.
Change-Id: Idd5fd7a3400fe65274d2a6343025e2ef8911635d
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/123518
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
Reviewed-by: Mike Kaganski <mike.kaganski@collabora.com>
|
|
The scenarios are:
1. Calling sequence's begin() and end() in pairs to pass to algorithms
(both calls use getArray(), which does the COW checks)
2. In addition to #1, calling end() again when checking result of find
algorithms, and/or begin() to calculate result's distance
3. Using non-const sequences in range-based for loops, which internally
do #1
4. Assigning sequence to another sequence variable, and then modifying
one of them
In many cases, the sequences could be made const, or treated as const
for the purposes of the algorithms (using std::as_const, std::cbegin,
and std::cend). Where algorithm modifies the sequence, it was changed
to only call getArray() once. For that, css::uno::toNonConstRange was
introduced, which returns a struct (sublclass of std::pair) with two
iterators [begin, end], that are calculated using one call to begin()
and one call to getLength().
To handle #4, css::uno::Sequence::swap was introduced, that swaps the
internal pointer to uno_Sequence. So when a local Sequence variable
should be assigned to another variable, and the latter will be modified
further, it's now possible to use swap instead, so the two sequences
are kept independent.
The modified places were found by temporarily removing non-const end().
Change-Id: I8fe2787f200eecb70744e8b77fbdf7a49653f628
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/123542
Tested-by: Jenkins
Reviewed-by: Mike Kaganski <mike.kaganski@collabora.com>
|
|
Change-Id: If50ae8156f81c1053aa8fbfc3148da64bb8e1442
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/111666
Reviewed-by: Michael Stahl <michael.stahl@allotropia.de>
Tested-by: Jenkins
|
|
Change-Id: I6ce64ca7c59639684779144ed0ed8d36c4aca32b
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/111665
Reviewed-by: Miklos Vajna <vmiklos@collabora.com>
Tested-by: Jenkins
|
|
Change-Id: Ia89059eea51ca396a7c74143625ac9a6706de198
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/120773
Tested-by: Jenkins
Reviewed-by: Ilmari Lauhakangas <ilmari.lauhakangas@libreoffice.org>
|
|
Issue the "instead of O[U]String, pass [u16]string_view" diagnostic also for
operator call arguments. (The "rather than copy, pass subView()" diagnostic is
already part of handleSubExprThatCouldBeView, so no need to repeat it explicitly
for operator call arguments.)
(And many call sites don't even require an explicit [u16]string_view, esp. with
the recent ad48b2b02f83eed41fb1eb8d16de7e804156fcf1 "Optimized OString operator
+= overloads". Just some test code in sal/qa/ that explicitly tests the
O[U]String functionality had to be excluded.)
Change-Id: I8d55ba5a7fa16a563f5ffe43d245125c88c793bc
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/115589
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>
|