Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
pmatilai commented on this pull request.
> +switch (num) {
+case 0:
+if (!bn) {
+bn = sig->bn = BN_new();
+}
+if (!bn) return 1;
+
+/* Create a BIGNUM from the signature pointer.
+ Note: this assumes big-endian data as required
+ by the PGP multiprecision integer format
+ (RFC4880, Section 3.2)
+ This will be useful later, as we can
+ retrieve this value with appropriate
+ padding. */
+bn = BN_bin2bn(p+2, mlen, bn);
+if (!bn) return 1;
Oh and FWIW, padding is a relatively rare condition. On my F25 laptop there are
3366 packages installed and 19 of them are "padding case", one of them being
(funny coincidence) that nss-softokn-3.28.1-1.0.fc25.x86_64.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/129___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
pmatilai commented on this pull request.
> +switch (num) {
+case 0:
+if (!bn) {
+bn = sig->bn = BN_new();
+}
+if (!bn) return 1;
+
+/* Create a BIGNUM from the signature pointer.
+ Note: this assumes big-endian data as required
+ by the PGP multiprecision integer format
+ (RFC4880, Section 3.2)
+ This will be useful later, as we can
+ retrieve this value with appropriate
+ padding. */
+bn = BN_bin2bn(p+2, mlen, bn);
+if (!bn) return 1;
NSS and beecrypt do need manual zero-padding, I don't know about openssl but
maybe its smarter. What I can tell you that the current code doesn't handle
padding correctly. Here's a reproducer package from the original NSS bug:
https://dl.fedoraproject.org/pub/archive/fedora/linux/releases/11/Fedora/x86_64/os/Packages/libuser-python-0.56.9-3.x86_64.rpm
but if you prefer a newer one, nss-softokn-3.28.1-1.0.fc25.x86_64 also
requires zero-padding (with NSS) and fails with the current openssl
implementation.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/129___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Allow SOURCE_DATE_EPOCH to override file timestamps (#144)
@bmwiedemann wrote: > The alternative approach would be to change the macro generating them and > first touch source .py files to set their date to $SOURCE_DATE_EPOCH so that > this gets embedded in .pyc headers I doubt that RPM can do this without delayed script execution because the bytecode generator is not necessarily installed when the %post runs. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/144#issuecomment-277920680___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
[Rpm-maint] [rpm-software-management/rpm] rpmkeys out of bounds heap read in pgpPrtSubType, rpmpgp.c line 444 (#148)
The attached file will cause an oud of bounds heap read in "rpmkeys -K". [rpmkeys-pgpPrtSubType-rpmpgp-444.zip](https://github.com/rpm-software-management/rpm/files/755884/rpmkeys-pgpPrtSubType-rpmpgp-444.zip) Here's the address sanitizer output: ``` ==15315==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60201a81 at pc 0x00677361 bp 0x7fff631cdeb0 sp 0x7fff631cdea8 READ of size 8 at 0x60201a81 thread T0 #0 0x677360 in pgpPrtSubType /f/rpm/rpm/rpmio/rpmpgp.c:444:3 #1 0x669d1d in pgpPrtSig /f/rpm/rpm/rpmio/rpmpgp.c:594:6 #2 0x669d1d in pgpPrtPkt /f/rpm/rpm/rpmio/rpmpgp.c:819 #3 0x669d1d in pgpPrtParams /f/rpm/rpm/rpmio/rpmpgp.c:978 #4 0x595487 in rpmSigInfoParse /f/rpm/rpm/lib/signature.c:104:6 #5 0x52d908 in rpmpkgVerifySigs /f/rpm/rpm/lib/rpmchecksig.c:263:7 #6 0x52f3ea in rpmcliVerifySignatures /f/rpm/rpm/lib/rpmchecksig.c:381:13 #7 0x50420d in main /f/rpm/rpm/rpmkeys.c:74:7 #8 0x7ff690a0078f in __libc_start_main (/lib64/libc.so.6+0x2078f) #9 0x41c558 in _start (/r/rpm/rpmkeys+0x41c558) 0x60201a81 is located 1 bytes to the right of 16-byte region [0x60201a70,0x60201a80) allocated by thread T0 here: #0 0x4cc6b8 in malloc (/r/rpm/rpmkeys+0x4cc6b8) #1 0x664624 in rmalloc /f/rpm/rpm/rpmio/rpmmalloc.c:44:13 -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/issues/148___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
sgallagher commented on this pull request.
> +switch (num) {
+case 0:
+if (!bn) {
+bn = sig->bn = BN_new();
+}
+if (!bn) return 1;
+
+/* Create a BIGNUM from the signature pointer.
+ Note: this assumes big-endian data as required
+ by the PGP multiprecision integer format
+ (RFC4880, Section 3.2)
+ This will be useful later, as we can
+ retrieve this value with appropriate
+ padding. */
+bn = BN_bin2bn(p+2, mlen, bn);
+if (!bn) return 1;
Mostly I was copying that from the beecrypt and NSS implementations, where the
signature was padded before comparison. Are you saying I can skip the padding
entirely at
https://github.com/rpm-software-management/rpm/pull/129/files/a7bc1e416e2f0f193be63317701bd3b18e2934b6#diff-763b93abc6d6f5549dafd08d9d07R523
?
This is definitely a place where I don't actually know what I'm doing...
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/129___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
sgallagher commented on this pull request.
> +EVP_PKEY_free(key->evp_pkey);
+key->evp_pkey = NULL;
+RSA_free(rsa);
+}
+
+return 1;
+}
+
+static int pgpSetKeyMpiRSA(pgpDigAlg pgpkey, int num, const uint8_t *p)
+{
+size_t mlen = pgpMpiLen(p) - 2;
+struct pgpDigKeyRSA_s *key = pgpkey->data;
+
+if(!key) {
+key = pgpkey->data = OPENSSL_secure_zalloc(sizeof(*key));
+if (!key) return 1;
I figured that, given that RPM interaction is a computationally-rare event, it
was easier on me to just assume everything should be secure-allocated rather
than trying to figure out when it was appropriate and when it was safe to skip
it.
If you are telling me that I should just not use it at all in this code, I'll
drop it.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/129___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
sgallagher commented on this pull request.
> +int DSA_SIG_set0(DSA_SIG *sig, BIGNUM *r, BIGNUM *s)
+{
+if (!sig) return 0;
+
+if (r) {
+sig->r = r;
+}
+
+if (s) {
+sig->s = s;
+}
+
+return 1;
+}
+#endif /* HAVE_DSA_SIG_SET0 */
+
I'm not really sure what you mean by "release" them? The way this function is
expected to work is that it updates any values that aren't NULL and does not
change any that are. Or at least, that's the way that the OpenSSL 1.1.0 version
implements them.
Also, these are "set" not "assigned", so it's not expected that this function
should be owning their memory management either. Meaning I shouldn't be calling
BN_free() before assigning the new value.
Am I way off-base here?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/129___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
t8m commented on this pull request.
> +int DSA_SIG_set0(DSA_SIG *sig, BIGNUM *r, BIGNUM *s)
+{
+if (!sig) return 0;
+
+if (r) {
+sig->r = r;
+}
+
+if (s) {
+sig->s = s;
+}
+
+return 1;
+}
+#endif /* HAVE_DSA_SIG_SET0 */
+
In above functions you do not release previous component BIGNUMs before
assigning new values. You should add that.
> +struct pgpDigKeyRSA_s *key = pgpkey->data;
+
+if(!key) {
+key = pgpkey->data = OPENSSL_secure_zalloc(sizeof(*key));
+if (!key) return 1;
+}
+
+switch(num) {
+case 0:
+/* Modulus */
+key->nbytes = mlen;
+/* Create a BIGNUM from the pointer.
+ Note: this assumes big-endian data. This
+ could be a bug on little-endian systems. */
+key->n = BN_bin2bn(p+2, mlen, NULL);
+if (!key->n) return 1;
You do not handle a theoretical double call with the same num on the same
pgpkey - it will leak memory. I do not know if this can happen (for example
with malformed data) though.
> +EVP_PKEY_free(key->evp_pkey);
+key->evp_pkey = NULL;
+RSA_free(rsa);
+}
+
+return 1;
+}
+
+static int pgpSetKeyMpiRSA(pgpDigAlg pgpkey, int num, const uint8_t *p)
+{
+size_t mlen = pgpMpiLen(p) - 2;
+struct pgpDigKeyRSA_s *key = pgpkey->data;
+
+if(!key) {
+key = pgpkey->data = OPENSSL_secure_zalloc(sizeof(*key));
+if (!key) return 1;
As the keys are public-only is it worth it using the secure memory allocator?
> +switch (num) {
+case 0:
+if (!bn) {
+bn = sig->bn = BN_new();
+}
+if (!bn) return 1;
+
+/* Create a BIGNUM from the signature pointer.
+ Note: this assumes big-endian data as required
+ by the PGP multiprecision integer format
+ (RFC4880, Section 3.2)
+ This will be useful later, as we can
+ retrieve this value with appropriate
+ padding. */
+bn = BN_bin2bn(p+2, mlen, bn);
+if (!bn) return 1;
It's unclear to me why do you convert the RSA signature to BIGNUM and then back
when calling the verification. Wouldn't it be better to just store the data
directly?
> +BIGNUM *r;
+BIGNUM *s;
+
+DSA_SIG *dsa_sig;
+};
+
+static int constructDSASignature(struct pgpDigSigDSA_s *sig)
+{
+int rc;
+
+if (sig->dsa_sig) {
+/* We've already constructed it, so just reuse it */
+return 1;
+}
+
+/* Create the DSA key */
Nitpick: you mean 'Create the DSA signature' here
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/129#pullrequestreview-20301305___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
@t8m Sorry, could you be more specific? I'm not sure which functions are risky or which references need to be freed. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/129#issuecomment-277728498___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
t8m requested changes on this pull request. The various _set0_ functions you add for compatibility with OpenSSL 1.0.2 do not properly handle multiple calls of the same function on the object - the old references are not freed. I understand that in the current patch this probably isn't a problem but it might happen in future. It might be better to release the old references before assigning new ones. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/129#pullrequestreview-20297097___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
@Conan-Kudo I just pushed a couple additional configure.ac changes that should ensure that it's using at least OpenSSL 1.0 -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/129#issuecomment-277719485___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
@sgallagher pushed 2 commits. de07401 fixup! Add initial OpenSSL support a7bc1e4 fixup! Add compatibility layer for OpenSSL 1.0.2 -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/129/files/d8e6b09913a4b383ad0dc0156e2539083f1fcb39..a7bc1e416e2f0f193be63317701bd3b18e2934b6 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
@Conan-Kudo Thanks, I'll see if I can make the configure checks a little more strict. Just to confirm: those other two bullet points were you asserting that things worked correctly, right? (It's a little confusing when corrective feedback is in the same list). -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/129#issuecomment-277678554___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] API documentation generation broken with doxygen > 1.8.7 (#131)
Closed #131. -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/issues/131#event-950123502___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] API documentation generation broken with doxygen > 1.8.7 (#131)
Oh. I suppose it even makes sense. Fixed now, by simply giving the Doxyheader files a .h extension (commit e8438555279b9a4e406ffd0edfc224de1c2862a6). Thank you for figuring this out! -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/issues/131#issuecomment-277652879___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
