Bug 1170162 - AUDIT-FIND: enlightenment: enlightenment_system: _store_umount_verify(): does not protect against shell metacharacters and relative path components
AUDIT-FIND: enlightenment: enlightenment_system: _store_umount_verify(): does...
Status: RESOLVED FIXED
Classification: openSUSE
Product: openSUSE Tumbleweed
Classification: openSUSE
Component: Security
Current
Other Other
: P5 - None : Normal (vote)
: ---
Assigned To: Simon Lees
E-mail List
:
Depends on:
Blocks: 1169238
  Show dependency treegraph
 
Reported: 2020-04-22 09:24 UTC by Matthias Gerstner
Modified: 2022-07-05 10:54 UTC (History)
2 users (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 Matthias Gerstner 2020-04-22 09:24:27 UTC
+++ This bug was initially created as a clone of Bug #1169238


This function tries to make sure that the user can only unmount his own mounts
below /media/$user. It also rejects backslashes in the path. However it does
not reject relative path components or shell characters.

- this allows a regular user to unmount arbitrary file systems by passing
  paths like "/media/$user/../../tmp.
- since the unmount is performed by calling the `umount` utility via
  "/bin/sh", shell metacharacters will be interpreted. Passing a path like
  '/media/testuser/$(date)' will cause the setuid-root program to execute the
  `date` program as root. This leads to full code execution as root. The only
  requirement is that a directory of the same name exists. Spaces are also
  allowed in the path, therefore even complex commands can be executed as root.

I recommend to reject relative path components and shell metacharacters in
this function to fix the issue.
Comment 1 Simon Lees 2020-04-22 10:50:41 UTC
Upstream: https://phab.enlightenment.org/T8671
Comment 2 Simon Lees 2020-04-23 07:35:14 UTC
Upstream Commit: https://phab.enlightenment.org/rE800ff4e24ff1a48dfa97f0cf6fe2d70c6768533b
Comment 3 Matthias Gerstner 2020-04-30 12:17:06 UTC
The upstream commit should fix the issue.

I would have liked to see some common logic between the different commands
instead of coding this complex query again. It's very hard to read...
Comment 4 Matthias Gerstner 2020-05-22 10:21:00 UTC
The umount has another possible attack surface. Even if the program only
allows removable devices to be mounted ... I could insert a removable device
with a UNIX file system on it that contains symlinks.

Then I could point the umount command to /media/$user/somemount/somelink and
the link target would be unmounted. The problem is that the `umount` program
is used for unmounting, which follows symlinks.

It would be better to use the `umount2` system call and pass "UMOUNT_NOFOLLOW"
to avoid symlinks being followed. Another approach could be to forbid slashes
after /media/$user/somemount and making sure that /media and /media/$user
aren't user controlled.

Can you approach upstream with this?
Comment 5 Simon Lees 2020-05-26 00:32:27 UTC
(In reply to Matthias Gerstner from comment #4)
> The umount has another possible attack surface. Even if the program only
> allows removable devices to be mounted ... I could insert a removable device
> with a UNIX file system on it that contains symlinks.
> 
> Then I could point the umount command to /media/$user/somemount/somelink and
> the link target would be unmounted. The problem is that the `umount` program
> is used for unmounting, which follows symlinks.
> 
> It would be better to use the `umount2` system call and pass
> "UMOUNT_NOFOLLOW"
> to avoid symlinks being followed. Another approach could be to forbid slashes
> after /media/$user/somemount and making sure that /media and /media/$user
> aren't user controlled.
> 
> Can you approach upstream with this?

I have approached upstream with these, I suspect the reason for not using the umount2 system call is that the mount / unmount section of this helper binary is only intended to be used as a fallback for systems with no udisks2 support ie bsd's, I suspect the safest fix is to conditionally remove the support at compile time when udisks is detected.
Comment 6 Matthias Gerstner 2020-07-08 11:51:18 UTC
Any news about the umount system call and upstream?
Comment 7 Simon Lees 2022-02-24 04:45:34 UTC
Following up here again sorry for the delay. The response from upstream was this is a fallback for systems that don't have udisks which is present on most Linux systems, so generally this code was designed to run on BSD systems which enlightenment does support. I don't believe most BSD systems include the umount2 syscall so it isn't really a solution.  

This code won't run on openSUSE so I can patch it out if it makes you more comfortable.
Comment 8 Matthias Gerstner 2022-02-24 10:14:14 UTC
Since the code, if run, would be a realistic security issue, as outlined in
comment 4, I would appreciate a patch that removes the code. If the fallback
is not relevant for Linux then it should not hurt apart from the burden of
carrying the patch.
Comment 10 Matthias Gerstner 2022-07-05 10:54:32 UTC
okay, thanks for addressing this. closing this bug as fixed.