nm: fix filename overflow for WiFi SSIDs with multi-byte characters#580
Open
rone-oliver wants to merge 4 commits into
Open
nm: fix filename overflow for WiFi SSIDs with multi-byte characters#580rone-oliver wants to merge 4 commits into
rone-oliver wants to merge 4 commits into
Conversation
SSIDs containing multi-byte UTF-8 characters (e.g. emojis) are percent-encoded by g_uri_escape_string(), expanding each byte to 3 characters. This causes the .nmconnection filename basename to exceed NAME_MAX (255 bytes), crashing NetworkManager with "File name too long". If the candidate basename exceeds NAME_MAX, replace the percent-encoded SSID with a SHA-256 hex digest of the raw SSID bytes. This guarantees a valid-length (91-byte), unique filename regardless of SSID content. Apply the same fix in netplan_get_id_from_nm_filepath() so the reverse lookup falls back to the hashed suffix when the escaped form is absent. Fixes: LP: #2147259
Add test_write_wifi_long_ssid_uses_hash() to test_netplan_nm.c to exercise the NAME_MAX overflow path in write_nm_conf_access_point(). A 20-emoji SSID in NM decimal-byte format (320 chars with semicolons) produces a 507-byte candidate basename when percent-encoded, exceeding NAME_MAX (255). The test verifies that _netplan_netdef_write_nm() falls back to a SHA-256 digest filename and that the basename is within NAME_MAX. Closes the coverage gap introduced by LP: #2147259 fix.
The 'long_ssid' ctests used raw UTF-8 emoji bytes, but
g_uri_escape_string() with allow_utf8=TRUE passes valid UTF-8
sequences through unescaped (80 bytes → 107-byte basename, well
under NAME_MAX). Those tests never triggered the SHA-256 hash
fallback, leaving lines 649-651 and 690-691 of util.c uncovered.
Change the SSID in both tests to the NM decimal-byte format
("240;159;152;128;" × 20). Semicolons encode to '%3B', yielding a
480-char escaped SSID and a 507-byte candidate basename > NAME_MAX,
which correctly exercises the hash path.
Also add the expected-hash assertion to
test_netplan_netdef_get_output_filename_nm_with_long_ssid to verify
the SHA-256 digest (not the escaped form) appears in the filename.
Fix a missing LCOV_EXCL_LINE on the chown() failure branch in
_netplan_g_string_free_to_file_with_permissions (consistent with the
surrounding getpwnam/getgrnam failure lines already marked).
Author
|
CI notes for reviewers: Coverage (Unit tests & Coverage): The overall C coverage shows ~96%, but this is a pre-existing baseline on main before this PR (the large gap is in gen-openvswitch.c, parse.c, etc., which are unrelated to this fix). The two files this PR touches — src/nm.c and src/util.c — are both at 100% coverage after the test commits. Autopkgtest / RPM build / Spread failures: These three checks are failing with the same errors on other recent merged PRs (infra/environment issues, not code regressions). They are not caused by this patch. |
…x/nm-ssid-filename-name-max-overflow
3 tasks
Author
|
I've documented the full reproduction steps and the technical impact of this bug on the end-user experience here: https://medium.com/@roneootan611/ubuntu-wi-fi-icon-disappearing-how-a-simple-emoji-can-crash-your-entire-network-stack-5dac8e8c5025. |
benhoyt
added
the
needs review soon
Author
|
Thanks for checking it out! Let me know if you need any context when you get a chance to review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
SSIDs containing multi-byte UTF-8 characters (e.g. emojis) are stored
by NetworkManager in a semicolon-delimited decimal-byte format
(e.g.
240;159;154;128). Wheng_uri_escape_string()percent-encodesthis string, each semicolon becomes
%3B, expanding a 20-emoji SSIDfrom ~160 bytes to ~507 bytes — well beyond the 255-byte NAME_MAX
limit. NetworkManager then crashes with "File name too long".
Fix: In
netplan_netdef_get_output_filename()and inwrite_nm_conf_access_point(), check the candidate basename lengthagainst
NAME_MAXbefore writing. If it would overflow, replace thepercent-encoded SSID with its SHA-256 hex digest (always 64 chars →
91-byte basename). The reverse lookup in
netplan_get_id_from_nm_filepath()is updated to fall back to thehashed suffix when the escaped form isn't found.
Two new ctests cover the normal path and the long-SSID hashing path.
Fixes LP: #2147259
Checklist
make checksuccessfully.make check-coverage).