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

pid1 serialization/deserialization fixes #10519

Merged
merged 8 commits into from Oct 26, 2018

Conversation

7 participants
@poettering
Copy link
Member

commented Oct 25, 2018

No description provided.

@evverx

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

This pull request introduces 1 alert when merging 134dbf5 into 4e412d2 - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

Comment posted by LGTM.com

if (k < 0)
log_error_errno(k, "Invalid line \"%s\": %m", line);
return k;

This comment has been minimized.

Copy link
@keszybz

keszybz Oct 25, 2018

Member

LGTM is right, the condition is borked here.

poettering added some commits Oct 17, 2018

core: when deserializing state always use read_line(…, LONG_LINE_MAX, …)
This should be much better than fgets(), as we can read substantially
longer lines and overly long lines result in proper errors.

Fixes a vulnerability discovered by Jann Horn at Google.

CVE-2018-15686
LP: #1796402
https://bugzilla.redhat.com/show_bug.cgi?id=1639071
core: enforce a limit on STATUS= texts recvd from services
Let's better be safe than sorry, and put a limit on what we receive.
automount: fix deserialization of dev_t
let's prefer "unsigned long" rather than "unsigned", in case there are
archs that have 32bit int, but 64bit dev_t.

(Also one cast was wrong anyway.)
core: strjoina() in a loop is never OK
Let's use plain strjoin() instead.
core: make manager_serialize() a bit easier to read by adding predica…
…te function

The predicate function manager_timestamp_shall_serialize() simply says
whether to serialize or not serialize a timestamp, and should make
things a bit easier to read.
core: rework serialization
Let's be more careful with what we serialize: let's ensure we never
serialize strings that are longer than LONG_LINE_MAX, so that we know we
can read them back with read_line(…, LONG_LINE_MAX, …) safely.

In order to implement this all serialization functions are move to
serialize.[ch], and internally will do line size checks. We'd rather
skip a serialization line (with a loud warning) than write an overly
long line out. Of course, this is just a second level protection, after
all the data we serialize shouldn't be this long in the first place.

While we are at it also clean up logging: while serializing make sure to
always log about errors immediately. Also, (void)ify all calls we don't
expect errors in (or catch errors as part of the general
fflush_and_check() at the end.

@poettering poettering force-pushed the poettering:serialize-fixes branch from 134dbf5 to b507423 Oct 26, 2018

@poettering

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2018

I force-pushed a new version now, that hopefully fixes the LGTM issue. No changes besides this, i.e. not changes outside of exec-util.c


- Don't use fgets(), it's too hard to properly handle errors such as overly
long lines. Use read_line() instead, which is our own function that handles
this much nicer.

This comment has been minimized.

Copy link
@evverx

evverx Oct 26, 2018

Member

I've just opened #10533, which should help to enforce this rule more or less automatically.

@keszybz keszybz merged commit 9f1c81d into systemd:master Oct 26, 2018

5 of 8 checks passed

bionic-amd64 autopkgtest running
Details
bionic-arm64 autopkgtest running
Details
bionic-i386 autopkgtest running
Details
Fedora Rawhide CI x86_64 rpm build [succeeded]
Details
LGTM analysis: C/C++ No alert changes
Details
LGTM analysis: JavaScript No code changes detected
Details
bionic-s390x autopkgtest finished (success)
Details
semaphoreci The build passed on Semaphore.
Details
@carnil

This comment has been minimized.

Copy link

commented Oct 27, 2018

CVE-2018-15686 was assigned for this issue

@ChenQi1989

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2018

@poettering
I'm doing backport to v239 to fix the CVE. I'd like to check with you whether I should backport all these 8 patches, or just the one that fixes the CVE.
In particular, the item 'rework serialization' worries me a little bit.

@ChenQi1989

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2018

I realize I might not express it clearly enough.
For CVE-2018-15686 alone, instead of the serialization/deserialization mechanism in systemd, is the following patch enough?
core: when deserializing state always use read_line(…, LONG_LINE_MAX, …)

@yuwata

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

@ChenQi1989 BTW, v239-stable branch has included this PR. https://github.com/systemd/systemd-stable/commits/v239-stable

@ChenQi1989

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2018

This is the first time that I know there exists a systemd-stable repo... Thanks for the info.
I have a question.
For each released version, e.g. v239, how long will it be maintained?

@anarcat

This comment has been minimized.

Copy link

commented Nov 13, 2018

@yuwata which commits, in particular, fix this issue in 239? is systemd/systemd-stable@1a05ff4 sufficient?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.