modif: Prevent sandbox name from containing only digits #5578
Conversation
There was a problem hiding this comment.
If I use only numbers for a name,
firejail --ls=numbers /fails. Is this an
actual bug? If you forbid only numbers in jails, I will close this PR.
From firejail(1):
--ls=name|pid dir_or_filename
List files in sandbox container, see FILE TRANSFER section for
more details.
A numeric argument is treated as the PID rather than the sandbox name.
See also the descriptions for --name= and --join=.
This is the behavior it shows, but it's explained nowhere that if a numeric argument is provided, it's treated as a PID. In fact you might prefer: if it's a numeric string, try to find it; if that PID does not exist, extract it (treat it as a normal name). I would do one of the following:
|
Yes, it should either be forbidden or at least be made clearer.
I think usability-wise, that would make it easier to join the wrong sandbox by
For the sake of simplicity, if I don't see the benefit of allowing numbers only since that precludes using the |
|
@kmk3 updated |
|
Regarding the commit message:
To make it clearer (and also in the style of git in the title), change it to
Or:
See the following for details: |
|
@kmk3 thank you for your review! Updated |
No problem :) By the way, the CI errors are apparently due to Azure: From https://github.com/netblue30/firejail/actions/runs/3905345101/jobs/6679876855: |
Names should not contain only numbers, as they are used in other commands as PIDs.
|
Merged, thanks for the patch! |
This amends commit 707f48a ("RELNOTES", 2023-02-14). Note: The "Allow only letters and digits" modif item was implemented on commit b4ffaa2 ("merges; more on cleaning up esc chars", 2023-02-14) and relates to both #5578 and #5613. The "--hostname" part of both the "Prevent" and the "Allow" modif items was also only added on that commit. Discussion about the hostname: #5613 (comment) Relates to #5578.
The "invalid_name" function claims to "allow strict ASCII letters and numbers". However, it uses isalnum(3) and isdigit(3), which may take the current locale into account and thus return 1 for non-ASCII characters. So add the following functions: * ascii_isalnum * ascii_isalpha * ascii_isdigit * ascii_isxdigit And use them in "invalid_name" so that it actually uses strictly ASCII in its comparisons. Added on commit b4ffaa2 ("merges; more on cleaning up esc chars", 2023-02-14). Relates to netblue30#5578. Kind of relates to netblue30#5708.
The "invalid_name" function claims to "allow strict ASCII letters and numbers". However, it uses isalnum(3) and isdigit(3), which may take the current locale into account and thus return 1 for non-ASCII characters. So add the following functions: * ascii_isalnum * ascii_isalpha * ascii_isdigit * ascii_isxdigit And use them in "invalid_name" so that it actually uses strictly ASCII in its comparisons. Added on commit b4ffaa2 ("merges; more on cleaning up esc chars", 2023-02-14). Relates to netblue30#5578. Kind of relates to netblue30#5708.
The "invalid_name" function claims to "allow strict ASCII letters and numbers". However, it uses isalnum(3) and isdigit(3), which may take the current locale into account and thus return 1 for non-ASCII characters. So add the following functions: * ascii_isalnum * ascii_isalpha * ascii_isdigit * ascii_isxdigit And use them in "invalid_name" so that it actually uses strictly ASCII in its comparisons. Added on commit b4ffaa2 ("merges; more on cleaning up esc chars", 2023-02-14). Relates to netblue30#5578. Kind of relates to netblue30#5708.
The "invalid_name" function claims to "allow strict ASCII letters and numbers". However, it uses isalnum(3) and isdigit(3), which may take the current locale into account and thus return 1 for non-ASCII characters. So add the following functions: * ascii_isalnum * ascii_isalpha * ascii_isdigit * ascii_islower * ascii_isupper * ascii_isxdigit And use the applicable ones in "invalid_name" so that it actually uses strictly ASCII in its comparisons. Added on commit b4ffaa2 ("merges; more on cleaning up esc chars", 2023-02-14). Relates to netblue30#5578. Kind of relates to netblue30#5708.
Changes: * Use only `invalid_name` to check the name and hostname instead of ad-hoc checks * Standardize empty/invalid error messages for name/hostname Note: This makes the hostname validation less strict, though it still forbids control characters and only numbers. Relates to netblue30#5578 netblue30#5708. See also commit b4ffaa2 ("merges; more on cleaning up esc chars", 2023-02-14).
Note that the sandbox name may also be set through the "join-or-start" option. Relates to netblue30#5578 netblue30#5708.
If I use only numbers for a name,
firejail --ls=numbers /fails. Is this an actual bug? If you forbid only numbers in jails, I will close this PR. I can rework it to delete theif's around the lines I added. This commit seems to me like the fix that touches the fewest lines of code. I gave priority toextract_pidwhich should better find the jail if it exists, is it correct?