Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

journalctl: verify that old entries are not sealed with too recent key #28885

Merged
merged 2 commits into from Oct 6, 2023

Conversation

felixdoerre
Copy link
Contributor

When verifying seals produced with forward secure sealing, the verification currently does not check that old entries are only sealed with the key for their epoch and not a more recent one. This missing check allows an attacker to remove seals, and create new ones with the currently available key, and verify will claim everything is in order, although all entries could have been modified. This PR adds the missing check. A proof-of-concept implementation to break and re-seal a journal is available here: https://github.com/kastel-security/Journald. With this patch applied the attack does not succeed anymore.

This change should be compatible as it only adjusts verification, so old (untampered) journals can be verified with the new journalctl, and "new" journals are identical and can be verified with the old journalctl.

This resolves CVE-2023-31439. See https://github.com/kastel-security/Journald/blob/main/journald-publication.pdf for more background.

See also #28433

@github-actions github-actions bot added journal please-review PR is ready for (re-)review by a maintainer labels Aug 18, 2023
When verifying seals produced with forward secure sealing, the verification
currently does not check that old entries are only sealed with the key for
their epoch and not a more recent one. This missing check allows an attacker
to remove seals, and create new ones with the currently available key, and
verify will claim everything is in order, although all entries could have
been modified.

This resolves CVE-2023-31439.
@YHNdnzj YHNdnzj added this to the v255 milestone Sep 9, 2023
@felixdoerre
Copy link
Contributor Author

Hi there, can someone find the time to review this? I still consider log sealing a security feature and consequently this issue a security vulnerability.

src/libsystemd/sd-journal/journal-verify.c Outdated Show resolved Hide resolved
src/libsystemd/sd-journal/journal-verify.c Outdated Show resolved Hide resolved
src/libsystemd/sd-journal/journal-verify.c Outdated Show resolved Hide resolved
src/libsystemd/sd-journal/journal-verify.c Outdated Show resolved Hide resolved
src/libsystemd/sd-journal/journal-verify.c Outdated Show resolved Hide resolved
src/libsystemd/sd-journal/journal-verify.c Outdated Show resolved Hide resolved
@yuwata yuwata added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Oct 3, 2023
@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Oct 3, 2023
@felixdoerre
Copy link
Contributor Author

Thank you for the review, I incorporated all suggestions. Regarding usec_t and usec_add: there are quite a few other timestamps in this method that still use plain addition and uint64_t, should I also adjust them to keep it consistent?

@yuwata yuwata added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed squash-on-merge and removed please-review PR is ready for (re-)review by a maintainer labels Oct 6, 2023
@yuwata
Copy link
Member

yuwata commented Oct 6, 2023

Next time, please squash commits, rather than appending commits to address comments.

@yuwata yuwata merged commit 3846d3a into systemd:main Oct 6, 2023
46 of 48 checks passed
@github-actions github-actions bot removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed squash-on-merge labels Oct 6, 2023
peckato1 pushed a commit to peckato1/systemd that referenced this pull request Jan 16, 2024
systemd#28885)

When verifying seals produced with forward secure sealing, the verification
currently does not check that old entries are only sealed with the key for
their epoch and not a more recent one. This missing check allows an attacker
to remove seals, and create new ones with the currently available key, and
verify will claim everything is in order, although all entries could have
been modified.

This resolves CVE-2023-31439.

Co-authored-by: Felix Dörre <felix.doerre@kit.edu>
(cherry picked from commit 3846d3a)
nmeyerhans pushed a commit to nmeyerhans/systemd that referenced this pull request Jan 21, 2024
systemd#28885)

When verifying seals produced with forward secure sealing, the verification
currently does not check that old entries are only sealed with the key for
their epoch and not a more recent one. This missing check allows an attacker
to remove seals, and create new ones with the currently available key, and
verify will claim everything is in order, although all entries could have
been modified.

This resolves CVE-2023-31439.

Co-authored-by: Felix Dörre <felix.doerre@kit.edu>
(cherry picked from commit 3846d3a)
(cherry picked from commit ea67d47)
(cherry picked from commit e140c1d)
yuwata pushed a commit to yuwata/systemd that referenced this pull request Apr 26, 2024
systemd#28885)

When verifying seals produced with forward secure sealing, the verification
currently does not check that old entries are only sealed with the key for
their epoch and not a more recent one. This missing check allows an attacker
to remove seals, and create new ones with the currently available key, and
verify will claim everything is in order, although all entries could have
been modified.

This resolves CVE-2023-31439.

Co-authored-by: Felix Dörre <felix.doerre@kit.edu>
(cherry picked from commit 3846d3a)
(cherry picked from commit ea67d47)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

4 participants