Race condition in multi-threaded use of archive_write_disk_header() on posix based systems

[xypiie]

Summary

the umask() call inside archive_write_disk_posix.c changes the umask of the whole process for a very short period of time and race condition with other thread can lead to permanent umask 0 setting:
_archive_write_disk_header(): umask(a->user_umask = umask(0));

process has umask of 027
thread1 enters _archive_write_disk_header()
thread1: prev_umask = umask(0) -> returns 027
thread1: whole process has now umask of 0
thread2 enters _archive_write_disk_header()
thread2: prev_umask = umask(0) -> returns 0
thread1: umask(prev_umask) sets to 027
thread2: umask (prev_umask) sets to 0
whole process remains at umask of 0

Effects (vulnerability)

Such race condition could lead to implicit directory creation with permissions 777 without sticky bit, which means any low privileged user on this system can delete and rename files inside those directories.
This is a serious vulnerability on a multi-user system and admins might not even be aware of it. They probably need to scan their full rootfs for such directories to mitigate.

Proposals

Short term: Adding info to README

Adding information to README to make users aware to mutex this call:
#1875

Potential solution for Linux (>= 4.7)

since linux kernel 4.7 rc1, the umask can be read from /proc fs without modifying it
see: torvalds/linux@3e42979

Example:
getumask.c

[xypiie]

CVE-2023-30571 has been assigned to this

[ljavorsk]

Hi, is there any update on the issue?

[ljavorsk]

@xypiie Could you please share more detail about your example? What it does do, what is expected behavior and what is current (faulty) behavior?
Thank you so much

[kientzle]

This issue arises if you are using multiple threads, each using libarchive to restore archive entries to disk. Note that libarchive has never officially supported this mode of use, primarily because libarchive relies on C library services that are not safe for multi-threaded use. Also note that umask() is not the only non-thread-safe function that libarchive relies on; fixing this issue would not be sufficient to support using libarchive from multiple threads simultaneously.

[theta682]

This issue arises if you are using multiple threads, each using libarchive to restore archive entries to disk. Note that libarchive has never officially supported this mode of use, primarily because libarchive relies on C library services that are not safe for multi-threaded use. Also note that umask() is not the only non-thread-safe function that libarchive relies on; fixing this issue would not be sufficient to support using libarchive from multiple threads simultaneously.

#1875 proposes to make a statement in the README file. I agree that it must be documented properly.

[xypiie]

This issue arises if you are using multiple threads, each using libarchive to restore archive entries to disk. Note that libarchive has never officially supported this mode of use, primarily because libarchive relies on C library services that are not safe for multi-threaded use. Also note that umask() is not the only non-thread-safe function that libarchive relies on; fixing this issue would not be sufficient to support using libarchive from multiple threads simultaneously.

In the README, it's stated: "The library is generally thread safe depending on the platform..." and adds some limits / constraints to it, like mentioning the usage of chdir(), which could cause problems.
From my point of view it's important to add information about the usage of the umask() calls as well, as this could even cause security issues. That's why I raised #1875 and from my point of view, merging it would close the case as then the users are informed and can take care about mutexing this call on their own. Further possibility and efforts to provide a fix inside libarchive code it is up to libarchive team to decide.

[kientzle]

Yeah, we should certainly clarify the documentation:

• The archive_read and archive_write core APIs are in fact generally thread safe, depending on how you implement the callbacks.
• The archive_read_disk and archive_write_disk APIs are not generally thread safe. Writing a thread-safe version of these APIs would be an interesting project, if anyone wanted to take on that challenge. (Hint: openat() and related *at() APIs are probably essential components for implementing this on POSIX.)

[arabesc]

This issue arises if you are using multiple threads, each using libarchive to restore archive entries to disk.

The issue does not require the use of libarchive from multiple threads. The problem is not in the libarchive itself, but in the thread-unsafe use of the system function umask that changes process global state. So, any code that uses umask from multiple threads without synchronization is unsafe, including the libarchive *disk* functions.

The archive_read_disk and archive_write_disk APIs are not generally thread safe.

It would be good to add in the README that the archive_*_disk_*() functions are API specific and that other APIs such as archive_write_new are safe. This will save time on investigating what it's all for and where it's used.

[arabesc]

I think a better solution than mutexing umask calls would be to ask a client to provide the mask value via a callback. There will be no umask calls in libarchive, no calls - no problem.