TRUNK-6549: Logging should not be vulnerable to injection attacks

[jwnasambu]

Description of what I changed

I have fixed the log injection vulnerability across several core files by sanitizing user controlled input before it is logged. This prevents attackers from forging log entries by injecting newline characters.

These is the overview of the changes I made:

• Created OpenmrsUtil.sanitizeForLogging which replaces carriage return (\r) and line feed (\n) characters with underscores (_).
• Sanitized systemId in becomeUser and the authenticated user in the logout log statement.
• Sanitized multiple log statements involving client names, system IDs, usernames, and privileges.
• Sanitized the username variable in the getUserByUsername method before it is written to the logs.

Issue I worked on

https://openmrs.atlassian.net/issues?filter=10641&selectedIssue=TRUNK-6549

Checklist: I completed these to help reviewers :)

• My IDE is configured to follow the code style of this project.

  No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend

• I have added tests to cover my changes. (If you refactored
  existing code that was well tested you do not have to add tests)

  No? -> write tests and add them to this commit git add . && git commit --amend

• I ran mvn clean package right before creating this pull request and
  added all formatting changes to my commit.

  No? -> execute above command

• All new and existing tests passed.

  No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.

• My pull request is based on the latest changes of the master branch.

  No? Unsure? -> execute command git pull --rebase upstream master

[codecov-commenter]

Codecov Report

❌ Patch coverage is 48.75000% with 41 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.24%. Comparing base (0a6d15c) to head (89e3ab6).

Files with missing lines	Patch %	Lines
...main/java/org/openmrs/api/context/UserContext.java	23.07%	13 Missing and 7 partials ⚠️
...main/java/org/openmrs/aop/AuthorizationAdvice.java	0.00%	8 Missing and 3 partials ⚠️
...pi/src/main/java/org/openmrs/util/OpenmrsUtil.java	77.77%	8 Missing ⚠️
...org/openmrs/api/db/hibernate/HibernateUserDAO.java	71.42%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #6165      +/-   ##
============================================
- Coverage     59.31%   59.24%   -0.07%     
- Complexity     9335     9349      +14     
============================================
  Files           695      695              
  Lines         37460    37501      +41     
  Branches       5517     5528      +11     
============================================
- Hits          22219    22218       -1     
- Misses        13242    13268      +26     
- Partials       1999     2015      +16     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[dkayiwa]

Code review — 9 findings

Reviewed the full diff at max effort (multi-angle finder pass, each finding independently verified against the PR head and current master). Ranked most-severe first; details are in the inline comments.

#	Severity	Location	Finding
1	medium	HibernateUserDAO:462/492/716	Three sibling user-influenced log sinks in this same file remain unsanitized
2	medium	OpenmrsUtil:2172	Blacklist misses other injection-capable chars (VT, FF, NEL, U+2028/9, ESC/ANSI)
3	low	UserContext:388	proxies list rendered raw in the same statement whose other arg was sanitized
4	low	UserContext:113	credentials.getAuthenticationScheme() arg left unsanitized
5	low	UserContext:171	Raw systemId concatenated into exception message that can reach logs
6	medium (design)	OpenmrsUtil:2168	A one-line %encode{%m}{CRLF} in the central log4j2 layout would sanitize every log site, past and future
7	low (perf)	UserContext:388	Eager sanitize defeats slf4j lazy {} on the AuthorizationAdvice hot path
8	low	OpenmrsUtil:2172	replaceAll recompiles a regex Pattern per call; literal replace is equivalent (suggestion attached)
9	low	OpenmrsUtil:2168	No unit test pins the new security primitive

Findings 3–5 are mechanism-real but currently need a module/custom-SPI caller to be exploitable; surfaced because the touched statements are exactly the ones being hardened.

🤖 Generated with Claude Code

[dkayiwa]

[medium] The fix is incomplete within this same file — three sibling user-influenced log sinks remain unsanitized:

• L462: log.info("updating password for {}", u.getUsername());
• L492: log.info("Updating secret question and answer for " + u.getUsername()); (string concatenation — worth converting to {} while here, like this line was)
• L716: log.debug("name: " + name); — the raw search string from the public UserService.getUsers(String name, …) API, the most directly user-typed value in this class.

username is settable via the public User.setUsername and name comes straight from the search API, so the same data sanitized here at L158 still forges log lines through those paths.

[dkayiwa]

[medium] The blacklist only covers \n/\r — other injection-capable characters pass through: vertical tab (U+000B), form feed (U+000C), NEL (U+0085), Unicode line/paragraph separators (U+2028/U+2029), backspace (U+0008), and ESC (U+001B). The CONSOLE appender writes %m to SYSTEM_OUT, so a terminal tailing the log will honor injected ANSI escape sequences (cursor movement, screen clearing, recoloring adjacent lines) — a residual log-injection vector that CR/LF stripping alone doesn't close.

Consider widening the strip, e.g. replaceAll("[\\p{Cntrl}\\u2028\\u2029&&[^\\t]]", "_"), or handling it once at the layout level with log4j2's purpose-built %encode{%m}{CRLF} (see the comment on the method signature).

[dkayiwa]

[low] This statement still logs unsanitized data through its second argument. proxies is rendered via List.toString(), and it holds the raw strings stored at L286 (proxies.add(privilege)) — the same values that get sanitized only for the log line at L285. Note the list can't be sanitized at insertion (equality in hasPrivilege/remove needs the real names), so wrap the rendering instead:

log.debug("Checking '{}' against proxies: {}", OpenmrsUtil.sanitizeForLogging(privilege), OpenmrsUtil.sanitizeForLogging(proxies));

(All in-tree addProxyPrivilege callers pass PrivilegeConstants.* constants today, so this needs a module passing tainted input to trigger — but since this exact statement is being hardened, the sibling arg should get the same treatment.)

[dkayiwa]

[low] The second argument is left unsanitized. credentials.getAuthenticationScheme() is rendered raw in the same statement whose first arg was just wrapped. In-repo implementations return constants (UsernamePasswordCredentials.SCHEME), but Credentials is a public SPI (since 2.3.0) and module implementations control that string — wrapping both args keeps the statement consistently hardened.

[dkayiwa]

[low] Four lines below, the same systemId goes raw into an exception message (L171):

throw new ContextAuthenticationException("User not found with systemId: " + systemId);

Exception messages routinely end up in logs via upstream catch-and-log handlers, so the value sanitized here at L167 can still reach the log unsanitized through getMessage(). Worth sanitizing there too while this method is being hardened.

[dkayiwa]

[medium, design] Consider fixing this once at the logging layer instead of per call site. Every OpenMRS appender already routes through a single pattern: defaultPattern in api/src/main/resources/log4j2.xml (L18, plus the webapp copy), mirrored by OpenmrsConstants.DEFAULT_LOG_LAYOUT_PATTERN and the log.layout global property. log4j 2.26.0 (current bom) ships both %replace{%m}{[\\r\\n]}{_} and the purpose-built %encode{%m}{CRLF} converters — one change there sanitizes every log statement, past and future, instead of 11 hand-picked arguments. Per-call-site wrapping is already incomplete in this PR (see the HibernateUserDAO comment) and nothing gates future log.* calls, so the vulnerability regrows over time.

Notes: stack traces are unaffected (emitted by the implicit throwable converter, not %m); installs with a persisted log.layout GP value would need a companion liquibase changeset; intentionally multi-line messages get flattened — which is the point of the defense. If the layout route is taken, this helper (and the 11 wraps) can be dropped entirely.

[dkayiwa]

[low, perf] Wrapping {} args defeats slf4j's lazy evaluation on a hot path. Previously this argument cost nothing when DEBUG was off (production default — org.openmrs.api is at INFO); now sanitizeForLogging(privilege) runs unconditionally, and String.replaceAll compiles a fresh regex Pattern on every call. hasPrivilege is invoked via AuthorizationAdvice for every @Authorized service method (AOPConfig binds the advice to every @Service class), and addProxyPrivilege/removeProxyPrivilege (L285/L308) run around those same calls — so this is pure waste on every authorization check, several times per service call.

Microseconds each, but avoidable: guard the hot sites with if (log.isDebugEnabled()), use the non-regex replace(char, char) form (see the comment on sanitizeForLogging), or move sanitization to the log4j2 layout where laziness is preserved automatically.

[dkayiwa]

[low] replaceAll takes a regex and compiles a fresh Pattern on every invocation — for a 2-character literal strip. String.replace(char, char) produces byte-identical output with no regex machinery, and returns this (zero allocation) when the char is absent, which is the overwhelmingly common case:

Suggested change

	return value.toString().replaceAll("[\n\r]", "_");
	return value.toString().replace('\n', '_').replace('\r', '_');

Two smaller notes: the javadoc says @return the sanitized string but null input returns null (fine for {} args — slf4j renders both as null — but String.valueOf(value) would make the method total and collapse the null branch); StringUtils.replaceChars(value.toString(), "\n\r", "__") is the commons-lang3 alternative already imported in this class.

[dkayiwa]

[low] No test pins the new security primitive. OpenmrsUtilTest already exists with the project's naming convention (e.g. url2file_shouldReturnNullGivenNullParameter). Worth adding e.g. sanitizeForLogging_shouldReplaceCarriageReturnAndLineFeedWithUnderscores and sanitizeForLogging_shouldReturnNullGivenNullParameter so a future refactor can't silently weaken the sanitizer — especially since the whole PR rests on this one-liner. (The PR checklist's tests box is unchecked.)

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud