Only use base 10 for numeric uids/gids #15991

keszybz · 2020-05-31T16:41:27Z

No description provided.

We would parse numbers with base prefixes as user identifiers. For example, "0x2b3bfa0" would be interpreted as UID==45334432 and "01750" would be interpreted as UID==1000. This parsing was used also in cases where either a user/group name or number may be specified. This means that names like 0x2b3bfa0 would be ambiguous: they are a valid user name according to our documented relaxed rules, but they would also be parsed as numeric uids. This behaviour is definitely not expected by users, since tools generally only accept decimal numbers (e.g. id, getent passwd), while other tools only accept user names and thus will interpret such strings as user names without even attempting to convert them to numbers (su, ssh). So let's follow suit and only accept numbers in decimal notation. Effectively this means that we will reject such strings as a username/uid/groupname/gid where strict mode is used, and try to look up a user/group with such a name in relaxed mode. Since the function changed is fairly low-level and fairly widely used, this affects multiple tools: loginctl show-user/enable-linger/disable-linger foo', the third argument in sysusers.d, fourth and fifth arguments in tmpfiles.d, etc. Fixes systemd#15985.

0x2b3bfa0 · 2020-05-31T19:08:20Z

src/test/test-user-util.c

        r = parse_uid("65535", &uid);
        assert_se(r == -ENXIO);
+        assert_se(uid == 100);
+
+        r = parse_uid("0x1234", &uid);
+        assert_se(r == -EINVAL);
+        assert_se(uid == 100);
+
+        r = parse_uid("01234", &uid);
+        assert_se(r == 0);
+        assert_se(uid == 1234);

        r = parse_uid("asdsdas", &uid);
        assert_se(r == -EINVAL);
+        assert_se(uid == 1234);


Suggested change

r = parse_uid("65535", &uid);

assert_se(r == -ENXIO);

assert_se(uid == 100);

r = parse_uid("0x1234", &uid);

assert_se(r == -EINVAL);

assert_se(uid == 100);

r = parse_uid("01234", &uid);

assert_se(r == 0);

assert_se(uid == 1234);

r = parse_uid("asdsdas", &uid);

assert_se(r == -EINVAL);

assert_se(uid == 1234);

r = parse_uid("65535", &uid);

assert_se(r == -ENXIO);

r = parse_uid("0x1234", &uid);

assert_se(r == -EINVAL);

r = parse_uid("01234", &uid);

assert_se(uid == 1234);

assert_se(r == 0);

r = parse_uid("asdsdas", &uid);

assert_se(r == -EINVAL);

I find rather confusing the unneeded checks to the uid value that remains from the last successful execution. Could we simply check the error if the function is expected to fail?

That's on purpose. It is testing that the function does not touch the output variable on failure.

Just a last question: would assigning UINT32_MAX to uid in case of error instead of leaving it untouched make any sense? It adds complexity to the code, but effectively avoids using a initialization value like 0 in case of error validation fault.

No. The convention is to not touch the output variable on error. In this case it doesn't matter much either way, but in other cases we make use of this. Having the same convention over all the code base makes the code easier to follow.

For example:

int output = DEFAULT_VALUE; r = parse_something(value_from_config_file, &output); if (r < 0) log_warning("Failed to parse..., ignoring"); ... r = parse_something(value_from_environment_variable, &output); if (r < 0) log_warning("Failed to parse..., ingorign"): ... use_value(output);

👌 Thank you for the clarification!

0x2b3bfa0 · 2020-05-31T19:16:48Z

Quote from #15987 (comment) and #15987 (comment):

I created a patch before I saw your pull request, see #15991.

Oh, in particular your patch rejects 01234, but id and other tools parse that as a decimal number. I think we should do the same, it would create a pointless difference to do anything different. So I'll close this in favour of #15991, since the implementation there is a bit nicer.

Thank you! Your implementation is definitely much nicer and includes tests.

By the way: does f7091f4 belong to this pull request?

keszybz · 2020-06-01T06:41:29Z

By the way: does f7091f4 belong to this pull request?

I put it there on purpose... I was looking at loginctl.c first, and noticed that this modernization can be done. We attach such cleanup patches to other PRs, because it's not worth to fire up the whole CI (which takes ~4 hours to run) for a trivial patch like that.

yuwata · 2020-06-01T08:04:49Z

LGTM.

poettering · 2020-06-01T10:44:35Z

i wonder if we should refuse uids beginning with "0" too, to avoid the ambiguity with octal?

keszybz · 2020-06-01T12:08:57Z

I thought about that, but id and others accept them. So I think it's better to not deviate from that.

0x2b3bfa0 · 2020-06-01T13:08:36Z

i wonder if we should refuse uids beginning with "0" too, to avoid the ambiguity with octal?

The original pull request (#15987) explicitly refused octal numbers, but as @keszybz stated, most of the existing tools ignore the leading zeros and parse any [0-9]+ string as base 10. Consistency here might be more important than strictness.

poettering · 2020-06-01T14:48:58Z

hmm, so "id" also accepts "+0" as alias for root, but not "-0"... which is super weird. Maybe it's not the best model to follow?

(we accept +0, too currently, and probably shouldn't, will prep patch for that)

poettering · 2020-06-01T14:51:32Z

and " 0" is accepted too (i.e. leading whitespace), and probably shouldn't either

0x2b3bfa0 · 2020-06-01T14:53:29Z

Good point. 🤔 I'm starting to think that #15987 wasn't such a bad idea after all.

keszybz · 2020-06-01T14:54:06Z

Yeah, we should not accept "+0" or "-0" or " 0".

poettering · 2020-06-01T14:54:26Z

i have a patch mostly ready for ding that

This allows disabling a few alternative ways to decode integers formatted as strings, for safety reasons. See: systemd#15991

poettering · 2020-06-01T17:07:01Z

I posted #16033 now. ptal

This allows disabling a few alternative ways to decode integers formatted as strings, for safety reasons. See: systemd#15991

This allows disabling a few alternative ways to decode integers formatted as strings, for safety reasons. See: systemd#15991 (cherry picked from commit 707e93a)

This allows disabling a few alternative ways to decode integers formatted as strings, for safety reasons. See: systemd#15991 CVE: CVE_2020-13776 Upstream-Status: Backport [systemd@707e93a] Signed-off-by: Dan Tran <dantran@microsoft.com>

This allows disabling a few alternative ways to decode integers formatted as strings, for safety reasons. See: systemd#15991 Backport of 707e93a

keszybz added 2 commits May 31, 2020 18:38

loginctl: define loop iterators in the loop header

f7091f4

keszybz mentioned this pull request May 31, 2020

Fix #15985 #15987

Closed

0x2b3bfa0 reviewed May 31, 2020

View reviewed changes

yuwata merged commit 397288e into systemd:master Jun 1, 2020

poettering added a commit to poettering/systemd that referenced this pull request Jun 1, 2020

parse-util: allow tweaking how to parse integers

30271af

This allows disabling a few alternative ways to decode integers formatted as strings, for safety reasons. See: systemd#15991

poettering mentioned this pull request Jun 1, 2020

various fixes and tweaks for integer parsing #16033

Merged

keszybz deleted the uids-gids-only-decimal branch June 2, 2020 10:14

poettering added a commit to poettering/systemd that referenced this pull request Jun 3, 2020

parse-util: allow tweaking how to parse integers

a4debbd

This allows disabling a few alternative ways to decode integers formatted as strings, for safety reasons. See: systemd#15991

poettering added a commit to poettering/systemd that referenced this pull request Jun 5, 2020

parse-util: allow tweaking how to parse integers

707e93a

This allows disabling a few alternative ways to decode integers formatted as strings, for safety reasons. See: systemd#15991

picnoir mentioned this pull request Jun 18, 2020

[20.03] systemd: apply patch for CVE-2020-13776 NixOS/nixpkgs#91048

Merged

10 tasks

Uh oh!

Only use base 10 for numeric uids/gids #15991

Only use base 10 for numeric uids/gids #15991

Uh oh!

Conversation

keszybz commented May 31, 2020

Uh oh!

0x2b3bfa0 May 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keszybz Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

0x2b3bfa0 Jun 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keszybz Jun 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0x2b3bfa0 Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

0x2b3bfa0 commented May 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keszybz commented Jun 1, 2020

Uh oh!

yuwata commented Jun 1, 2020

Uh oh!

poettering commented Jun 1, 2020

Uh oh!

keszybz commented Jun 1, 2020

Uh oh!

0x2b3bfa0 commented Jun 1, 2020

Uh oh!

poettering commented Jun 1, 2020

Uh oh!

poettering commented Jun 1, 2020

Uh oh!

0x2b3bfa0 commented Jun 1, 2020

Uh oh!

keszybz commented Jun 1, 2020

Uh oh!

poettering commented Jun 1, 2020

Uh oh!

poettering commented Jun 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

0x2b3bfa0 May 31, 2020 •

edited

Loading

0x2b3bfa0 Jun 1, 2020 •

edited

Loading

keszybz Jun 1, 2020 •

edited

Loading

0x2b3bfa0 commented May 31, 2020 •

edited

Loading

poettering commented Jun 1, 2020 •

edited

Loading