Problem/Motivation
Both value and name accept internal control characters (notably \n, \r, \t) at storage time. Any consumer that materializes multiple SSH keys into a single \n-delimited blob (e.g. to build an authorized_keys file) then sees the embedded newline split a single field's content across two lines — the second of which can be attacker-controlled.
Two related paths get to the same vulnerability:
1. name is treated as a free text label, but has no content constraint.
SshKeyItem::getConstraints() enforces Length::max = 128 on name but nothing about what characters it may contain. The default sshkey_textarea widget renders name as an <input type="textfield">, so a browser user can't easily enter a newline — but JSON:API, REST, migration, and programmatic setValue() all bypass that. A name of "laptop\nssh-ed25519 AAA… backdoor@host" stored via any of those paths flows through a downstream consumer's "<alg> <key> <name>"-style rebuild and becomes two lines in authorized_keys, the second of which is a fully attacker-supplied SSH entry.
2. value's comment portion accepts internal control chars even when the key portion doesn't.
The constraint validator does base64_decode($key, TRUE) in strict mode, which rejects \n / \r / \t inside the base64-encoded key bytes. But Utils::initialize() parses with explode(' ', $value, 3), so everything after the second space is the comment — including any embedded newline. Validation passes (algorithm OK, key OK, structure OK, phpseclib loads it), and then onChange() derives name from the dirty comment via Utils::getComment(). The result is the same injection vector as path (1), now via a single field on the value side.
Issue #3592555 (already in 4.x) trims leading/trailing whitespace from value; this issue is about internal control characters in both value and name.
Steps to reproduce
- Apply a sshkey field to a fieldable entity.
- Via JSON:API (or any programmatic save path), set the field with:
value: a valid SSH key — e.g."ssh-ed25519 AAAA… legit@host".name:"laptop\nssh-ed25519 BBBB… attacker@host"(containing a literal newline).
- Save. The entity validates and persists because the only
nameconstraint is length. - Build an
authorized_keysblob from this and any other sshkey entries withimplode("\n", …)(or hand the multi-key string toansible.posix.authorized_key): the attacker line lands in the file as a separate authorized entry.
Alternative reproduction via the value path: store value = "ssh-ed25519 AAAA… legit-comment\nssh-ed25519 BBBB… attacker" and leave name unset. Validation passes (the key portion is clean base64), and onChange() derives name from Utils::getComment(), which contains the embedded newline.
Proposed resolution
Normalize on write in SshKeyItem::onChange():
valuebranch: after the existing trim, replace any internal\r/\n/\twith a single space. Regular spaces are preserved so legitimate comments survive (e.g."Colan's mobile laptop"). Write the normalized value back viawritePropertyValue()before fingerprint/name derivation, so derived properties see the clean string.namebranch (new): whennameis set (explicitly or via the value-derived path), collapse any whitespace run to a single space and trim.nameis a short single-line label; internal newlines or tabs are never meaningful here.
This is defense-in-depth alongside the existing constraint validator — validation still runs and still rejects malformed keys; normalization just guarantees that whatever gets stored is downstream-safe regardless of entry point (widget, JSON:API, REST, migration, programmatic setValue).
Remaining tasks
- Normalize internal control chars in
valueinonChange(). - Normalize
nameinonChange()(both the explicitly-set and value-derived paths). - Kernel-test coverage for both paths, plus a regression test using the injection payload from the reproduction.
- Maintainer review.
- Tag a release (the same release that ships #3592555).
API changes
None. value and name are still strings; they just no longer carry internal control characters.
Data model changes
None at the schema level. Existing rows with bad internal whitespace will normalize on their next save. A hook_update_N() sweep is possible but probably not warranted — the bug only manifests when a downstream consumer is doing newline-joining, and any such consumer should be doing its own defensive sanitization too.
Comments
Comment #2
colanComment #4
colan