Re: [Rpm-maint] [rpm-software-management/rpm] Add OpenSSL crypto backend (#129)
pmatilai 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;
FWIW, rpm only ever deals with public keys, signing is done with gpg.
--
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.
> +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 Panu says - as there are no secret/private key operations at all, there is
no point in using the secure memory in the patch at all.
--
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 */
+
No, I mean when you assign the sig->r values you overwrite the previous values
that could be there which means the previous value will be leaked. If rpm
guarantees that pgpSet(Key|Sig)Mpi* functions won't be called on the same
key/sig twice with the same index it is not strictly necessary to handle this,
because you will have the previous value always set to NULL. But I do not know
whether rpm really guarantees that when reading some malformed signature/key
data.
--
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.
> +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;
I'd probably try removing the conversion to BIGNUM and back if it solves the
padding problem. If not then it needs to be investigated how to handle the
padding properly.
--
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] rpmkeys out of bounds heap read in pgpPrtSubType, rpmpgp.c line 444 (#148)
Thanks for the report, fixed in commit 657553ffabd29f63d5c8b42a8284fa524d21be19. This needs backporting to older versions too (so keeping open for now) -- 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#issuecomment-277955447___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] heap out of bounds read in copyTdEntry() (#133)
Fixed in commit 3a07ba3ba6f2c7d594730beefe8235b7eba4af52. This one needs backporting to older versions so keeping open for now. -- 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/133#issuecomment-277955656___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] invalid read in dataLength / grabData (header.c) (#138)
The package is no longer readable after commit 3a07ba3ba6f2c7d594730beefe8235b7eba4af52 because there's a negative tag involved. However the actual crash here is due to RPMTAG_PREFIXES type mismatch (int32 in package, assumed string array) combined with lack of validation and error checking - rpm assumes tags are of correct type almost everywhere throughout the codebase. Sigh. -- 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/138#issuecomment-277958357___ 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)
mlschroe 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;
beecrypt needs manual zero-padding? Where in the code would that be?
Anyway, the current code doesn't do padding as sig->len is used as length. If
openssl needs padding (which it seems to do), you must use
EVP_PKEY_size(pkey_ctx) instead.
--
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;
Oh, my recollection of beecrypt requiring padding is from some ancient comment
by somebody, somewhere, so either I'm misremembering, things have changed since
then or the commenter was wrong. Or any combination of the three. Sorry, didn't
intend to spread misinformation.
--
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] rpmkeys out of bounds heap read in pgpPrtSubType, rpmpgp.c line 444 (#148)
Just for completeness: Here's a different file triggering an out of bounds a few lines earlier. It seems it is fixed by the same commit (sidenote: I think it'd be a good idea to have regression tests with all the fuzzed files that triggered bugs). [rpmkeys-oob-heap-pgpPrtSubType-rpmpgp-427.zip](https://github.com/rpm-software-management/rpm/files/757334/rpmkeys-oob-heap-pgpPrtSubType-rpmpgp-427.zip) asan message (from a 4.13.0 compile): ``` ==27208==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602019bd at pc 0x00677a6a bp 0x7ffe5597dc70 sp 0x7ffe5597dc68 READ of size 4 at 0x602019bd thread T0 #0 0x677a69 in pgpPrtSubType /f/rpm/rpm-4.13.0/rpmio/rpmpgp.c:427:3 #1 0x66a45d in pgpPrtSig /f/rpm/rpm-4.13.0/rpmio/rpmpgp.c:594:6 #2 0x66a45d in pgpPrtPkt /f/rpm/rpm-4.13.0/rpmio/rpmpgp.c:819 #3 0x66a45d in pgpPrtParams /f/rpm/rpm-4.13.0/rpmio/rpmpgp.c:978 #4 0x592c67 in rpmSigInfoParse /f/rpm/rpm-4.13.0/lib/signature.c:90:6 #5 0x52d789 in rpmpkgVerifySigs /f/rpm/rpm-4.13.0/lib/rpmchecksig.c:270:7 #6 0x52f19a in rpmcliVerifySignatures /f/rpm/rpm-4.13.0/lib/rpmchecksig.c:388:13 #7 0x50415d in main /f/rpm/rpm-4.13.0/rpmkeys.c:70:7 #8 0x7f36453fb78f in __libc_start_main (/lib64/libc.so.6+0x2078f) #9 0x41c4a8 in _start (/f/rpm/rpm-4.13.0/rpmkeys+0x41c4a8) 0x602019bd is located 0 bytes to the right of 13-byte region [0x602019b0,0x602019bd) allocated by thread T0 here: #0 0x4cc608 in malloc (/f/rpm/rpm-4.13.0/rpmkeys+0x4cc608) #1 0x664d64 in rmalloc /f/rpm/rpm-4.13.0/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#issuecomment-277964994___ 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 bound heap read in pgpPrtSig, rpmpgp.c:533 (#149)
The attached file triggers an out of bounds heap read in rmpkeys -K. [rpmkeys-heap-oob-pgpPrtSig-rpmpgp-533.zip](https://github.com/rpm-software-management/rpm/files/757347/rpmkeys-heap-oob-pgpPrtSig-rpmpgp-533.zip) asan error with current git (you get more meaningful ones with ASAN_OPTIONS="fast_unwind_on_malloc=0"): ``` ==23681==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60201a80 at pc 0x0066c870 bp 0x7fff5c578470 sp 0x7fff5c578468 READ of size 1 at 0x60201a80 thread T0 #0 0x66c86f in pgpPrtSig /f/rpm/rpm/rpmio/rpmpgp.c:533:23 #1 0x66c86f in pgpPrtPkt /f/rpm/rpm/rpmio/rpmpgp.c:823 #2 0x66c86f in pgpPrtParams /f/rpm/rpm/rpmio/rpmpgp.c:982 #3 0x595487 in rpmSigInfoParse /f/rpm/rpm/lib/signature.c:104:6 #4 0x52d908 in rpmpkgVerifySigs /f/rpm/rpm/lib/rpmchecksig.c:263:7 #5 0x52f3ea in rpmcliVerifySignatures /f/rpm/rpm/lib/rpmchecksig.c:381:13 #6 0x50420d in main /f/rpm/rpm/rpmkeys.c:74:7 #7 0x7f9783a7378f in __libc_start_main (/lib64/libc.so.6+0x2078f) #8 0x41c558 in _start (/r/rpm/rpmkeys+0x41c558) 0x60201a80 is located 0 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 #2 0x5d0677 in copyTdEntry /f/rpm/rpm/lib/header.c:1096:12 #3 0x5cf8e4 in headerNext /f/rpm/rpm/lib/header.c:1712:7 #4 0x52d310 in rpmpkgVerifySigs /f/rpm/rpm/lib/rpmchecksig.c:262:12 #5 0x52f3ea in rpmcliVerifySignatures /f/rpm/rpm/lib/rpmchecksig.c:381:13 #6 0x50420d in main /f/rpm/rpm/rpmkeys.c:74:7 #7 0x7f9783a7378f in __libc_start_main (/lib64/libc.so.6+0x2078f) #8 0x41c558 in _start (/r/rpm/rpmkeys+0x41c558) -- 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/149___ 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;
Beecrypt is definitely doing some sort of padding:
https://github.com/sgallagher/rpm/blob/openssl/rpmio/digest_beecrypt.c#L288
--
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;
OK, I will drop the secure memory entirely. I was being overcautious.
--
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 */
+
OK, I think I finally see what you mean. I was thrown off because the comment
was associated with the code where it's assigned to the final object (which can
only happen once), but the issue was before this. I will fix 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.
> +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;
@mlschroe Thanks, I wasn't entirely sure where to get the correct padding
length. I will try that and see if it addresses the example package @pmatilai
provided.
--
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)
I believe I have addressed all of the review comments thus far. I also ran the code through a Coverity static analysis, which came up clean. -- 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-278201836___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] rpmkeys out of bound heap read in pgpPrtSig, rpmpgp.c:533 (#149)
Thanks for the report, fixed in commit 4ab3e0c5d1538cd35e106c4ecae3497048ad9763. This too needs backporting... -- 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/149#issuecomment-278243463___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
