@DemiMarie pushed 1 commit.
8f0c8600f1bc25dd9b724ee4d4086fc0bf91827c Check that count and data length are
reasonable
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
@DemiMarie pushed 1 commit.
4acff44a2f438921445ecb93f7d85e781292f0a3 Reject signatures in immutable headers
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
We actually already reject empty headers (with no tags at all) in
`hdrblobVerifyRegion`. So the only question remaining is if an empty region
(`ril == 0`) is valid.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
This check will probably need to be moved to the package reading functions.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
Programs like DNF assume that RPM checks all signatures for validity, but
signatures outside the signature header won’t be checked. Therefore, they must
be rejected.
You can view, comment on, or merge this pull request online at:
https://github.com/rpm-software-management/rpm/pull/1503
--
@DemiMarie pushed 1 commit.
781dba2b45dc9c3ac3825630ac1ce2f2d34b8451 ‘hdrblobInit’: check pointer is
8-byte aligned
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
The following gzipped RPM package will demonstrate the bug on an RPM built with
UBSan.
[rpm-4.15.1-3.fc32.1.src.rpm.gz](https://github.com/rpm-software-management/rpm/files/5820367/rpm-4.15.1-3.fc32.1.src.rpm.gz)
--
You are receiving this because you are subscribed to this thread.
Reply to
> For heavens sake. All along I've asking to make available the reproducer
> cases that you DO HAVE. Nothing else.
Sorry; this was a misunderstanding on my part. Uploaded in the other thread.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or
> All I've been asking is for you to make available the reproducers that you do
> have.
Ah okay. I thought you were asking me to make reproducers for all of them,
which would take time I don’t really have right now. Here is an RPM (gzipped
so GitHub will accept it) that reproduces the
I did manage to add two fuzz targets, one of which is in one these PRs. The
other was submitted privately to Red Hat Security Response as it found some
significant security problems
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on
> I still haven't seen a single reproducer. Please make them available for the
> cases you have them - like said I need to prioritize. I initially thought the
> newly created individual PR's were those, but clearly that's not the case.
I can create a reproducer for the integer overflow which
> Um, seems I wasn't quite awake yesterday. There's no universal law that says
> that every pointer must be 8-byte aligned. Alignment depends on the
> architecture, pointer sizes and all. Like I said, refer to the thing that the
> alignment depends on, ie blob->ie. It's size and alignment is
> Also note that what dnf and friends do is only header signature verification,
> they do not actually verify the payload at all. They verify that the download
> matches what was in the repository, but that's not the same as being
> untampered with. Also, you do not want rpmReadPackageFile() to
@DemiMarie pushed 1 commit.
1eb4725e92a00fbcc27caead5a788d70515d2a6b ‘hdrblobInit’: check pointer is
8-byte aligned
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
@DemiMarie pushed 1 commit.
24fa3475dea6f393f3fd088e83970b392d6c6348 Avoid incrementing a pointer past the
end
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
@DemiMarie pushed 1 commit.
fc4d264682aa89bcd61a941a4e328eb2c0df59f3 Check that count and data length are
reasonable
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
@DemiMarie pushed 1 commit.
3319d5e0f18848da0da43ee71a45ee694fff04c0 Forbid tag data with count zero
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
@DemiMarie pushed 1 commit.
f6e9d5487b456a0808fba41ed3db9cd0f5c07a3b Verify that data does not overlap
region trailer
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
@DemiMarie pushed 1 commit.
a47f8d59fe831f02faa4e4f8f51e99deab8d0e99 Forbid tag data with count zero
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
> But that's getting off track. The thing is, there can never be "only one" set
> of algorithms in rpm. The initial design did just that, and that's why we're
> still forced to deal with MD5 as a required field in packages produced a
> decade after MD5 was declared obsolete. The rpm lifespan
The only case where `end` can be beyond the allocation is for the last entry in
the header. This can happen for v3 headers, or v4 headers that aren’t
contiguous. I know that compilers are allowed to assume that the arguments to
`memcpy` can be dereferenced, and the same may also be true of
@DemiMarie pushed 1 commit.
e02bf8213191e53ddde98171ab2ba8900c33833b A header with count zero has length
zero
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
Negating INT32_MIN is undefined behavior, so we must prevent it.
You can view, comment on, or merge this pull request online at:
https://github.com/rpm-software-management/rpm/pull/1502
-- Commit Summary --
* Avoid negating an attacker-controlled signed integer
-- File Changes --
M
We want to remove as many forms of undefined behavior as we can. This
adds flags to make integer and pointer overflows well-defined.
Furthermore, it turns on strong stack protection.
You can view, comment on, or merge this pull request online at:
This is already checked for other headers.
You can view, comment on, or merge this pull request online at:
https://github.com/rpm-software-management/rpm/pull/1498
-- Commit Summary --
* Verify that data does not overlap region trailer
-- File Changes --
M lib/header.c (5)
-- Patch
Functions in the public RPM API use hdrblobInit() to import an RPM
header from memory, but that skips critical checks. Fix this by not
skipping these checks.
You can view, comment on, or merge this pull request online at:
https://github.com/rpm-software-management/rpm/pull/1500
-- Commit
This prevents integer overflows.
You can view, comment on, or merge this pull request online at:
https://github.com/rpm-software-management/rpm/pull/1497
-- Commit Summary --
* Check that len is in range before using it
-- File Changes --
M lib/header.c (8)
-- Patch Links --
Otherwise, we will dereference a misaligned pointer, which is undefined
behavior.
You can view, comment on, or merge this pull request online at:
https://github.com/rpm-software-management/rpm/pull/1499
-- Commit Summary --
* ‘hdrblobInit’: check pointer is 8-byte aligned
-- File Changes
Such headers are useless and might cause problems elsewhere.
You can view, comment on, or merge this pull request online at:
https://github.com/rpm-software-management/rpm/pull/1495
-- Commit Summary --
* Forbid headers with only a region
-- File Changes --
M lib/header.c (6)
--
Previously we would suffer an integer underflow in this case.
You can view, comment on, or merge this pull request online at:
https://github.com/rpm-software-management/rpm/pull/1496
-- Commit Summary --
* A header with count zero has length zero
-- File Changes --
M lib/header.c (3)
This is already checked in regionSwab() but it is better to check it
earlier, in case someone uses hdrblobInit() without hdrblobImport().
You can view, comment on, or merge this pull request online at:
https://github.com/rpm-software-management/rpm/pull/1493
-- Commit Summary --
* Check
This ensures adding ‘REGION_TAG_COUNT’ to it will not overflow.
You can view, comment on, or merge this pull request online at:
https://github.com/rpm-software-management/rpm/pull/1494
-- Commit Summary --
* Check that ‘einfo.offset’ is reasonable
-- File Changes --
M lib/header.c (3)
You can view, comment on, or merge this pull request online at:
https://github.com/rpm-software-management/rpm/pull/1488
-- Commit Summary --
* Clean up rdl calculation
-- File Changes --
M lib/header.c (4)
-- Patch Links --
You can view, comment on, or merge this pull request online at:
https://github.com/rpm-software-management/rpm/pull/1490
-- Commit Summary --
* Add a fuzz target for ‘headerImport’
* Fuzz headerExport() too
-- File Changes --
M lib/header.c (50)
-- Patch Links --
The ‘end’ parameter to ‘strtaglen’ might point past the end of an
allocation. Therefore, if ‘start’ becomes equal to ‘end’, return an
error without calling ‘memchr’ on that pointer.
You can view, comment on, or merge this pull request online at:
This avoids any possible integer overflows.
You can view, comment on, or merge this pull request online at:
https://github.com/rpm-software-management/rpm/pull/1492
-- Commit Summary --
* Check that count and data length are reasonable
-- File Changes --
M lib/header.c (3)
-- Patch
This avoids a potential out-of-bounds read in dataLength().
You can view, comment on, or merge this pull request online at:
https://github.com/rpm-software-management/rpm/pull/1491
-- Commit Summary --
* Check that type and length are not out of range
-- File Changes --
M lib/header.c
> Please split off the commits with actual reproducer to a separate PR, and
> make the reproducers available someplace. That allows us to prioritize,
> properly reviewing this kind of stuff is a lot of work.
Will do. I only have reproducers for a small subset of these, though.
> Second, split
@pmatilai so here is what I would *like* to see:
1. Duplicate tags in signature headers are not allowed.
2. Signature headers are not allowed to duplicate tags found in the immutable
header, after fixups.
3. Signatures are verified before headers are imported.
4. `rpmReadPackageFile` is
@pmatilai Taking a step back, the purpose of this ticket is to ensure that were
a vulnerability found in RPM’s header manipulation, it would not be possible to
exploit it. There are two paths that could help here:
1. Fuzz `rpmReadPackageFile` and friends.
2. Reduce the amount of code that
`rpmReadPackageFile()` currently ignores the transaction verification level,
forcing clients such as DNF and libdnf to check after the fact if signatures
were present. Respecting the verification level in `rpmReadPackageFile` would
be cleaner. If backwards-compatibility precludes this, we
I have not been able to determine if librpm is thread-safe. From looking at
the code, it appears not to be, for several reasons:
- librpm changes global state, such as the process umask.
- Lua scripts can change the environment, which can race with access to the
environment from other threads.
@Conan-Kudo That is fantastic news!
--
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/1482#issuecomment-758096633___
Rpm-maint
@DemiMarie commented on this pull request.
> + lua_settop(L, 1);
+ rc = rpm_expand(L);
+ }
+}
+return rc;
+}
+
+static int mc_newindex(lua_State *L)
+{
+rpmMacroContext *mc = checkmc(L, 1);
+const char *name = luaL_checkstring(L, 2);
+if (lua_isnil(L,
@mlschroe tell that to the Fedora infrastructure maintainers. They don’t sign
their metadata.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
The LGTM alert is a false positive: the dead code is a static assertion, so it
is a no-op at runtime anyway.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
@DemiMarie commented on this pull request.
> @@ -169,8 +169,8 @@ rpmRC rpmpkgRead(struct rpmvs_s *vs, FD_t fd,
goto exit;
}
-/* Read the signature header. Might not be in a contiguous region. */
-if (hdrblobRead(fd, 1, 0, RPMTAG_HEADERSIGNATURES, sigblob, ))
+/* Read
Fast and has a massive security margin. I believe the best known attacks are
on 3 rounds vs 12, and libsodium has a hyper-optimized SIMD implementation it
uses for Argon2.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
Some of the advantages of this approach:
- The initial hash covers the entire package, and does not need to be updated
when signatures are added or removed.
- Multiple signatures are automatically supported.
- Signatures are timestamped and can expire.
- Key fingerprints include the algorithm as
That’s understandable. Ideally, this blob would be as simple as possible; the
current signature blob is more complicated than necessary. What about a
Blake2b hash of the lead+header+payload, followed by a list of (length,
timestamp, expiration, Blake2b hash of (algorithm ID||public key), raw
@DemiMarie commented on this pull request.
> + lua_settop(L, 1);
+ rc = rpm_expand(L);
+ }
+}
+return rc;
+}
+
+static int mc_newindex(lua_State *L)
+{
+rpmMacroContext *mc = checkmc(L, 1);
+const char *name = luaL_checkstring(L, 2);
+if (lua_isnil(L,
For RPMv6, we can replace the signature header with detached signatures. To
quote [my comment on another issue]:
> I am strongly in favor of detached signatures, for multiple reasons:
>
> * Detached signatures can be verified without having to parse the RPM _at
> all_. This dramatically
@pmatilai we can also drop support for *parsing* v3 packages, which will help
reduce our attack surface.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
I am strongly in favor of detached signatures, for multiple reasons:
- Detached signatures can be verified without having to parse the RPM *at all*.
This dramatically reduces the attack surface ― only the PGP signature parser
and the crypto code remains.
- Detached signatures can be verified
@DemiMarie commented on this pull request.
> @@ -169,8 +169,8 @@ rpmRC rpmpkgRead(struct rpmvs_s *vs, FD_t fd,
goto exit;
}
-/* Read the signature header. Might not be in a contiguous region. */
-if (hdrblobRead(fd, 1, 0, RPMTAG_HEADERSIGNATURES, sigblob, ))
+/* Read
@DemiMarie commented on this pull request.
> @@ -169,8 +169,8 @@ rpmRC rpmpkgRead(struct rpmvs_s *vs, FD_t fd,
goto exit;
}
-/* Read the signature header. Might not be in a contiguous region. */
-if (hdrblobRead(fd, 1, 0, RPMTAG_HEADERSIGNATURES, sigblob, ))
+/* Read
@DemiMarie commented on this pull request.
> @@ -169,8 +169,8 @@ rpmRC rpmpkgRead(struct rpmvs_s *vs, FD_t fd,
goto exit;
}
-/* Read the signature header. Might not be in a contiguous region. */
-if (hdrblobRead(fd, 1, 0, RPMTAG_HEADERSIGNATURES, sigblob, ))
+/* Read
When working on #1471 I found that the invariants of the trailer are not
documented anywhere I could find. I expected them to be as follows:
- The trailer must come after the last index entry in the contiguous region.
- The trailer must not overlap with any other header entry, whether in the
@Conan-Kudo good point on optional dependencies.
That said, if we are going to make Lua mandatory, could we use it for the PGP
packet parser? Lua is de-facto memory safe, so the risk of nasty security
vulnerabilities is far lower, and performance should not matter for this
application. And
The signature verification code, and all other code that interacts with a
package before the signature has been verified, is security critical. It
should be fuzz tested as much as possible.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or
@DemiMarie commented on this pull request.
> @@ -169,8 +169,8 @@ rpmRC rpmpkgRead(struct rpmvs_s *vs, FD_t fd,
goto exit;
}
-/* Read the signature header. Might not be in a contiguous region. */
-if (hdrblobRead(fd, 1, 0, RPMTAG_HEADERSIGNATURES, sigblob, ))
+/* Read
> @DemiMarie : this is an excellent point. There is verification of the whole
> rpm file in librepo (see
> [rpm-software-management/librepo#222](https://github.com/rpm-software-management/librepo/pull/222))
> and rpm signature verification is done after that, but there remains the
>
@DemiMarie commented on this pull request.
> @@ -169,8 +169,8 @@ rpmRC rpmpkgRead(struct rpmvs_s *vs, FD_t fd,
goto exit;
}
-/* Read the signature header. Might not be in a contiguous region. */
-if (hdrblobRead(fd, 1, 0, RPMTAG_HEADERSIGNATURES, sigblob, ))
+/* Read
@DemiMarie pushed 5 commits.
5774a927b28c2056aed9bd9fc3e39605e0b071de Enable hardening flags where available
2b86b349a2f66f28a66080ddc9d8f6e76f9acbfb Check that len is in range before
using it
69a79e4a240e6b82d537d6fc76b57ace55a0e17a Avoid incrementing a pointer past the
end
@Conan-Kudo done
--
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/1471#issuecomment-752321926___
Rpm-maint mailing list
@DemiMarie commented on this pull request.
> @@ -169,8 +169,8 @@ rpmRC rpmpkgRead(struct rpmvs_s *vs, FD_t fd,
goto exit;
}
-/* Read the signature header. Might not be in a contiguous region. */
-if (hdrblobRead(fd, 1, 0, RPMTAG_HEADERSIGNATURES, sigblob, ))
+/* Read
How will package signatures be verified? More specifically, will `rpm2extents`
verify the signed digest of files before decompressing them? Otherwise, this
seems like a potential security risk, in case there is a bug in the
decompression library.
--
You are receiving this because you are
This makes vulnerabilities less likely by:
- Preventing an out-of-bounds read on 32-bit systems.
- Adding `-fno-strict-overflow`, `-fwrapv`, and `-fwrapv-pointer`
- Avoid some undefined pointer arithmetic
- Requiring signature headers to be contiguous.
You can view, comment on, or merge this pull
Currently, `rpm -K` parses the header as well as the signature. If it only
parsed the signature, the attack surface would be much smaller, as a far
simpler parser could be used.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on
> That probably provides no material benefit for us. IRIX, AIX, and other
> Unix-types are supported by community contributors. OS/2 support is
> maintained _mostly_ out of tree, but we don't need to make their lives
> considerably harder if we don't have to.
It wouldn’t be a regression,
> > > > > > > Yes, this is a known - or not so well known - limitation. As the
> > > > > > > signature check is basically done by hand it lack a lot of
> > > > > > > feature one would expect of GPG proper.
> > > > > >
> > > > > >
> > > > > > Can we (as an option) use a third-party library,
> > > > > Yes, this is a known - or not so well known - limitation. As the
> > > > > signature check is basically done by hand it lack a lot of feature
> > > > > one would expect of GPG proper.
> > > >
> > > >
> > > > Can we (as an option) use a third-party library, such as
> > > >
> @DemiMarie, is there any reason to use your lib instead of sequoia?
Sequoia is GPL; not sure if this is a problem. I have no affiliation with
rpgp; it is merely a Rust library I came across.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly
A knob that defaults to off would be fine.
--
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/1467#issuecomment-751378608___
> > > > > Besides the currently obsolete things, new things need to be built
> > > > > with the mindset that all crypto _will_ become obsolete over time,
> > > > > and avoid putting it into new places where it only gets in our way
> > > > > eventually.
> > > >
> > > >
> > > > I suggest
> > > Yes, this is a known - or not so well known - limitation. As the
> > > signature check is basically done by hand it lack a lot of feature one
> > > would expect of GPG proper.
> >
> >
> > Can we (as an option) use a third-party library, such as [rpgp](/rpgp/rpgp)?
>
> Rust is not
That said, there are C libraries that we can use instead, such as the one used
by Thunderbird.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
Hash functions with outputs smaller than 224 bits, and <2048 bit RSA and DSA
signatures, are not a good idea. RPM should refuse to rely on such algorithms
for security.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
> > > Besides the currently obsolete things, new things need to be built with
> > > the mindset that all crypto _will_ become obsolete over time, and avoid
> > > putting it into new places where it only gets in our way eventually.
> >
> >
> > I suggest avoiding algorithm agility as much as
Much of the complexity in PKCS#7, PKCS#12, and OpenPGP comes from being too
flexible.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
IMO, moving from OpenPGP to PKCS#7 would hardly be a victory. Moving to
something like Signify would.
Ideally, the signature would be at a fixed offset and of a fixed length, so
there is no need to parse the file before checking the signature. That
eliminates an enormous class of
> Yes, this is a known - or not so well known - limitation. As the signature
> check is basically done by hand it lack a lot of feature one would expect of
> GPG proper.
Can we (as an option) use a third-party library, such as [rpgp](/rpgp/rpgp)?
--
You are receiving this because you are
> Besides the currently obsolete things, new things need to be built with the
> mindset that all crypto _will_ become obsolete over time, and avoid putting
> it into new places where it only gets in our way eventually.
I suggest avoiding algorithm agility as much as possible. It is great in
`rpm --eval 1 >/dev/full; echo $?` shows 0 and no error message.
--
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/1444___
Indeed it is, but not being able to write `quote_array` is somewhat concerning,
especially since `\x1a` is technically a valid character in filenames.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
If a macro is passed several arguments separated by some whitespace, I expect
that `%{**}` includes the original whitespace. Instead, this whitespace is
lost.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
We can vastly expand what can be done from Lua by using LuaJIT and its
fantastic FFI.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
Right now, I cannot write a Lua function `quote_array` that converts an array
of strings to a string that (when used as the argument to a macro) will be
interpreted as the initial array. This is because \\x1a (ASCII unit separator)
cannot be escaped.
One solution would be to allow \\x1a to be
`%{lua:print "\0a"}` expands to the empty string, which is almost certainly not
what the programmer intended. Since NUL characters aren’t allowed in macro
expansions, RPM should emit an error in this case.
--
You are receiving this because you are subscribed to this thread.
Reply to this
401 - 489 of 489 matches
Mail list logo
