Bug 1202725 - AUDIT-1: CVE-2022-40673: kdiskmark: adjust whitelisting, follow-up review
AUDIT-1: CVE-2022-40673: kdiskmark: adjust whitelisting, follow-up review
Status: RESOLVED FIXED
Classification: openSUSE
Product: openSUSE Tumbleweed
Classification: openSUSE
Component: Security
Current
Other Other
: P5 - None : Normal (vote)
: ---
Assigned To: Matthias Gerstner
E-mail List
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2022-08-24 17:48 UTC by Dmitry Sidorov
Modified: 2022-09-22 12:31 UTC (History)
1 user (show)

See Also:
Found By: ---
Services Priority:
Business Priority:
Blocker: ---
Marketing QA Status: ---
IT Deployment: ---


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Sidorov 2022-08-24 17:48:27 UTC
Last year I requested a kdiskmark review: https://bugzilla.opensuse.org/show_bug.cgi?id=1182521
It was successfully resolved.

But now I have changed the namespace from org.jonmagon to dev.jonmagon (because I am a person, not an organization). And besides, I want to request a re-review, because the code has changed and now not only clearing the cache, but also the testing itself is done through the helper. The helper is started with KAuth, and opens a new DBus connection.

The fio name is hard-coded in the helper. An user pass numeric parameters and a string file name. I check the file name to make sure that at least there is no block device specified. I hope there is no possibility of any CVE here.

The package is placed in benchmark:kdiskmark

[  110s] kdiskmark.x86_64: I: polkit-cant-acquire-privilege dev.jonmagon.kdiskmark.helper.init (??:no:auth_admin_keep)

[  110s] kdiskmark.x86_64: W: polkit-untracked-privilege dev.jonmagon.kdiskmark.helper.init (??:no:auth_admin_keep)

[  110s] kdiskmark.x86_64: E: suse-dbus-unauthorized-service (Badness: 10000) /usr/share/dbus-1/system-services/dev.jonmagon.kdiskmark.helper.service

I'd be grateful for any replacements.
Comment 1 Dmitry Sidorov 2022-08-24 17:52:01 UTC
The last word is feedback, not replacements. Sry :)
Comment 2 Matthias Gerstner 2022-08-25 08:20:45 UTC
Thank you for involving us. We will schedule the review and adjust the
whitelisting accordingly when done.
Comment 3 Matthias Gerstner 2022-08-31 10:05:31 UTC
I will look into this.
Comment 4 Matthias Gerstner 2022-08-31 13:41:56 UTC
I am afraid that this new design of the kdiskmark D-Bus interface is not
acceptable for security reasons.

There are three main D-Bus ingredients:

1) The dev.jonmagon.kdiskmark.helper Kauth interface with an authenticated
  "init" method. When this is successfully invoked then the actual helper
  described next is launched (see Helper::init()).
2) The Helper object on the dev.jonmagon.kdiskmark.helper interface. This is
  where the actual logic for benchmarking is implemented. it has methods like
  prepareFile(), removeFile() and startTest().
3) The Benchmark *client* also implements an interface called
  "dev.jonmagon.kdiskmark.applicationinterface". This seems to serve the only
  purpose to signal to the helper described in 2) when the client
  disappears. The helper then shuts down as well.

The current approach is strange in that the service and the client are
monitoring each other and shut down if their respective peer goes away. It
makes some sense for the client to monitor the service, because it cannot
operate without it, but the service should normally not do that the other way
around. The service could decide to shutdown after a certain timeout if there
are no more active client sessions but this can be monitored without the
client firing up their own D-Bus interface, the service only needs to monitor
the active D-Bus client connections it has.

Security wise only the part described in 1) is authenticated. This means that
the *actual* helper service can only be launched after *somebody*
authenticated as admin. After that the helper described in 2) is happily
accessible to *anybody* in the system that has access to the D-Bus system bus.
This can re reproduced pretty easily:

- start the graphical kdiskmark tool and correctly enter the admin password.
  This will trigger the kauth service described in 1) to fire up the actual
  helper service described in 2).
- now run a shell as an arbitrary other user, e.g. nobody:

  # become nobody to simulate a local attacker
  root# sudo -u nobody /bin/bash
  # this will remove the file /etc/fstab without further authentication
  nobody$ gdbus call -y -d dev.jonmagon.kdiskmark.helper -o /Helper -m dev.jonmagon.kdiskmark.helper.removeFile /etc/fstab

The available D-Bus methods implemented in the helper described in 2) allow
for the following security issues:

- removeFile(): allows to remove arbitrary files in the system with root
  privileges.
- prepareFile(): allows to create arbitrarily large files in arbitrary
  locations on the file system as root. There is a restriction to the path,
  the final path component needs to end in ".kdiskmark.tmp". Via directory
  symlinks this can also be adjusted somewhat. In any case it allows local
  disk space DoS and maybe more if the creation of a file owned by root grants
  additional privileges in other software.
- flushPageCache(): drops all file system caches on the kernel side, kind of a
  local performance DoS.
- startTest() similar to prepareFile(), but it of course also generates a lot
  of I/O load. Arbitrary code execution luckily is not possible, because most
  input parameters are integers and not strings and the command line for the
  `fio` tool is carefully constructed in a way that no parameter injection is
  possible.

Furthermore there are DoS vectors against kdiskmark itself:

- anybody can start/stop the test.
- anybody can own the dev.jonmagon.kdiskmark.applicationinterface service on
  the system bus, thereby making it impossible for other users in the system
  to start kdiskmark successfully.

So to implement this properly I am suggesting the following:

- the service should be normally autostarted without an init function that
  needs to be authenticated. That is the usual way to implement the helper.
- every single D-Bus method in the helper that allows to escalate privileges
  needs to be authenticated via a Kauth action.
- if you want to avoid entering the password multiple times for multiple
  actions then you can group together multiple actions via the Polkit imply
  annotation (see [1], org.freedesktop.policykit.imply).

  [1]: https://www.freedesktop.org/software/polkit/docs/latest/polkit.8.html
- if you have issues with the limited time the authentication is cached by
  polkit itself then you can try an approach like kpmcore, although I'am not
  an outright fan of it. It was discussed in bug 1178848 comment 4 "the
  service explicitly caches authorization for clients indefinitely".
- "applicationinterface" should be dropped, if it is only used for signaling
  whether the client is still there, as I suppose. The service can count the
  number of active client connections itself.
- since this helper service can currently only support a single benchmark at
  the same time it should prevent multiple sessions being started. This can be
  coupled with the connection counting logic. If a client connection is already
  active then further connections can simply be rejected. A proper session
  management could also be done using something like an "initSession()" that
  is authenticated once, multiple sessions are rejected via a proper error
  message, all other D-Bus methods are only accepted if the same client that
  properly authenticated initSession() calls them.

If an affected version of kdiskmark is already publicly released then we
should also follow formal security issue handling procedures:

- define a time window until the issues are fixed and after which the findings
  will be made public.
- define which CVEs should be assigned and who requests them.

See also our full disclosure policy [2].

[2]: https://en.opensuse.org/openSUSE:Security_disclosure_policy
Comment 5 Dmitry Sidorov 2022-08-31 18:39:18 UTC
Matthias, thank you so much for such a detailed explanation. I take your comments into account, will try to solve these things.
Comment 6 Dmitry Sidorov 2022-09-07 09:55:15 UTC
Hi. I looked at the helper work in kpm and did a similar thing.
What exactly have I done:
- Migrated to Polkit, KAuth cannot handle arbitrary connection names
- Authorization occurs only in the Helper::initSession function, the authorization is cached here. The rest of the helper's actions only check for cached authorization.
- If one connection is already established (or rather authorization cached), all other connections will be rejected, helper.cpp:85
- No symbolic links can be accepted, helper.cpp:131
- The file path is taken only once in Helper::prepareFile, functions Helper::startBenchmarkTest and Helper::removeBenchmarkFile can only work with the file with which testing was started.
- Once a file name is set, it cannot be changed in the current session. Helper::endSession may have been called, or the client application may have been closed of course.
- Thus, a temporary polkit authorization will not interrupt the testing after 5 minutes (or revoked by pkcheck --revoke-temp). But a new session will require a new authorization if 5 minutes have already passed.

Could you please make another review basing on current state of benchmark:kdiskmark?
Comment 7 Matthias Gerstner 2022-09-12 08:56:06 UTC
Yes I will look into the new source code.

So was the previous version of the tool already publicly released?
Comment 8 Dmitry Sidorov 2022-09-12 11:15:40 UTC
> So was the previous version of the tool already publicly released?

Yes, it was. Now "suppressed" by the new version.
Comment 9 Matthias Gerstner 2022-09-12 12:03:16 UTC
The new code is looking better now. For prudence I suggest the following
change in Helper::initSession():

-    PolkitQt1::Authority::Result result = PolkitQt1::Authority::No;
+    PolkitQt1::Authority::Result result;

Did you also package the vulnerable version already on other distributions?
Comment 10 Dmitry Sidorov 2022-09-12 13:47:50 UTC
> Did you also package the vulnerable version already on other distributions?

For other distributions I do not package. According to https://repology.org/project/kdiskmark/versions 3.0.0 is in the suppression process for Fedora (Rawhide) and OpenMandriva (Cooker), but 3.0.0 is also still in FreeBSD ports

As for the suggested changes: do you mean to explicitly initialize result? In line 92 it's just the way it's suggested.
Comment 11 Matthias Gerstner 2022-09-13 08:34:22 UTC
(In reply to jonmagon@gmail.com from comment #10)
> For other distributions I do not package. According to https://repology.org/project/kdiskmark/versions 3.0.0 is in the suppression process for Fedora (Rawhide) and OpenMandriva (Cooker), but 3.0.0 is also still in FreeBSD ports

Then it is better to request a CVE for the affected version and I will write a
post on the oss-sec mailing list to make other integrators aware. I can
request a CVE from Mitre for this.

> As for the suggested changes: do you mean to explicitly initialize result? In line 92 it's just the way it's suggested.

Yes that's what I mean. Sorry I got the diff in comment 9 reversed.
Comment 12 Dmitry Sidorov 2022-09-13 09:36:58 UTC
> Yes that's what I mean. Sorry I got the diff in comment 9 reversed.

I figured as much, changed it in master, I agree it makes sense to make it more explicit.

> I can request a CVE from Mitre for this.

If this would encourage them to update the package, that would be nice.
Comment 13 Matthias Gerstner 2022-09-13 12:46:36 UTC
So I started the whitelisting process for version 3.1.1 and the adjusted
naming scheme.

I also requested a CVE from Mitre for the "failure to protect D-Bus methods".
Comment 14 OBSbugzilla Bot 2022-09-13 15:45:03 UTC
This is an autogenerated message for OBS integration:
This bug (1202725) was mentioned in
https://build.opensuse.org/request/show/1003348 Factory / rpmlint
Comment 15 Matthias Gerstner 2022-09-14 09:05:15 UTC
I received CVE-2022-40673 from Mitre to track the issue of insufficient D-Bus
method authentication in kdiskmark version 3.0.0. You should also mention this
CVE in your release notes or wherever you see fit.

I'm publishing this bug now since the fixed version is already public, too.

The whitelisting for version 3.1.1 is also on its way to Factory by now. Thus
we can soon close this bug. I will write a small posting on the oss-security
mailing list to describe this issue for the rest of the community.
Comment 16 Dmitry Sidorov 2022-09-14 09:25:53 UTC
Thank you. I have added a warning to the release description.
Comment 17 OBSbugzilla Bot 2022-09-14 09:35:04 UTC
This is an autogenerated message for OBS integration:
This bug (1202725) was mentioned in
https://build.opensuse.org/request/show/1003484 Factory / rpmlint
Comment 18 Matthias Gerstner 2022-09-14 10:14:10 UTC
The post to oss-security is out:

https://www.openwall.com/lists/oss-security/2022/09/14/1

With this I'm closing this bug as fixed.
Comment 19 Dmitry Sidorov 2022-09-16 15:28:27 UTC
Probably something else is missing:

[   60s] kdiskmark.x86_64: E: polkit-untracked-privilege (Badness: 10000) dev.jonmagon.kdiskmark.helper.init (no:no:auth_admin_keep)
Comment 20 Dmitry Sidorov 2022-09-16 15:34:38 UTC
(In reply to Dmitry Sidorov from comment #19)
> Probably something else is missing:
> 
> [   60s] kdiskmark.x86_64: E: polkit-untracked-privilege (Badness: 10000)
> dev.jonmagon.kdiskmark.helper.init (no:no:auth_admin_keep)

logfile: https://build.opensuse.org/build/openSUSE:Factory/standard/x86_64/kdiskmark/_log
Comment 21 Matthias Gerstner 2022-09-19 08:37:19 UTC
(In reply to jonmagon@gmail.com from comment #20)
> (In reply to Dmitry Sidorov from comment #19)
> > Probably something else is missing:
> > 
> > [   60s] kdiskmark.x86_64: E: polkit-untracked-privilege (Badness: 10000)
> > dev.jonmagon.kdiskmark.helper.init (no:no:auth_admin_keep)
> 
> logfile: https://build.opensuse.org/build/openSUSE:Factory/standard/x86_64/kdiskmark/_log

Ah, indeed, I overlooked the changed name of the Polkit action. I will take
care of it.
Comment 22 OBSbugzilla Bot 2022-09-19 11:45:05 UTC
This is an autogenerated message for OBS integration:
This bug (1202725) was mentioned in
https://build.opensuse.org/request/show/1004679 Factory / polkit-default-privs
Comment 23 Matthias Gerstner 2022-09-22 12:31:16 UTC
Something strange seems to have happened there in the build service, the
submission should never have been accepted to Factory without the Polkit
whitelisting. It still did somehow, but the RPMs never built what it looks
like.

Anyway, the polkit whitelisting is there now and the build in Factory is now
succeeding. Closing again.