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

  1. Apply a sshkey field to a fieldable entity.
  2. 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).
  3. Save. The entity validates and persists because the only name constraint is length.
  4. Build an authorized_keys blob from this and any other sshkey entries with implode("\n", …) (or hand the multi-key string to ansible.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():

  • value branch: after the existing trim, replace any internal \r / \n / \t with a single space. Regular spaces are preserved so legitimate comments survive (e.g. "Colan's mobile laptop"). Write the normalized value back via writePropertyValue() before fingerprint/name derivation, so derived properties see the clean string.
  • name branch (new): when name is set (explicitly or via the value-derived path), collapse any whitespace run to a single space and trim. name is 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 value in onChange().
  • Normalize name in onChange() (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

colan created an issue. See original summary.

colan’s picture

  • colan committed e5388439 on 4.x
    Issue #3592563 by colan: Normalize internal control characters in value...
colan’s picture

Status: Active » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.