cgivre opened a new pull request, #3041: URL: https://github.com/apache/drill/pull/3041
Potential fix for [https://github.com/apache/drill/security/code-scanning/22](https://github.com/apache/drill/security/code-scanning/22) At a high level, the fix is to prevent any user-controlled string from being used as a format string to `String.format` or similar APIs. Instead, the formatting helper should either (a) always treat the input as literal text, or (b) if formatting with arguments is desired, use a constant format string that incorporates the user data via `%s`, not as the pattern itself. The single best fix here is to change `UserException.Builder.message(String format, Object... args)` so that it no longer treats the first parameter as a format string. We can preserve the public API (callers still pass `message("x %s y", arg)`), but internally we will build the message by concatenating the base text and the argument representations instead of re-invoking `String.format`. This completely eliminates the externally-controlled format-string sink while keeping behavior close enough for error reporting. Concretely, in `common/src/main/java/org/apache/drill/common/exceptions/UserException.java`, in the `Builder.message` method, we will replace: ```java if (args.length == 0) { message = format; } else { message = String.format(format, args); } ``` with logic that, when `args.length > 0`, converts `format` and `args` into a single string without using `String.format` on untrusted input, e.g., `format + " " + Arrays.toString(args)`. This keeps existing functionality (message still contains both the base text and argument values) and removes the vulnerable call. No other files need code changes for this specific alert; the upstream flows into this method are then harmless, because the method no longer treats the parameter as a format string. Implementation details: - Edit only within `UserException.Builder.message` in `UserException.java`. - Add an import for `java.util.Arrays` at the top of the file, as we will use `Arrays.toString(args)` to render the argument list. - Ensure no other behavior of `UserException` is changed (we preserve the wrapping semantics, context handling, etc.). - No modifications are required in `BaseOptionManager`, `QueryResources`, `QueryWrapper`, `RestQueryRunner`, or `StatusResources` for this specific CWE; once the sink is safe, those taint flows are no longer exploitable. --- _Suggested fixes powered by Copilot Autofix. Review carefully before merging._ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
