[xabt] Validate CustomJniInitFunctions names are valid C identifiers#10935
[xabt] Validate CustomJniInitFunctions names are valid C identifiers#10935jonathanpeppers merged 1 commit intomainfrom
Conversation
Reject AndroidStaticJniInitFunction items that are not valid C identifiers before they reach the LLVM IR generator. A malicious NuGet package could inject arbitrary LLVM IR directives via newlines or special characters in function names (CWE-74). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the NativeAOT build pipeline by validating @(AndroidStaticJniInitFunction) (passed into CustomJniInitFunctions) so that only valid C identifiers are accepted, preventing potential LLVM IR injection via crafted item specs.
Changes:
- Add C-identifier validation (and newline rejection) for
CustomJniInitFunctionsbefore generating LLVM IR. - Add a NativeAOT release build test ensuring invalid function names fail the build with an appropriate error.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/Xamarin.Android.Build.Tasks/Tasks/GenerateNativeAotLibraryLoadAssemblerSources.cs |
Validates custom JNI init function names before feeding them into the LLVM IR generator. |
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest2.cs |
Adds a regression test covering rejection of invalid custom JNI init function names. |
src/Xamarin.Android.Build.Tasks/Tasks/GenerateNativeAotLibraryLoadAssemblerSources.cs
Show resolved
Hide resolved
src/Xamarin.Android.Build.Tasks/Tasks/GenerateNativeAotLibraryLoadAssemblerSources.cs
Show resolved
Hide resolved
jonathanpeppers
left a comment
There was a problem hiding this comment.
🤖 AI Review Summary
Verdict: ✅ LGTM
Clean, well-targeted security fix (CWE-74 injection via LLVM IR) with a good regression test. No issues found.
👍 Positives:
- Security: The
ValidCIdentifierregex +IndexOfAnynewline check provides defense-in-depth against LLVM IR injection. The newline check is technically redundant (the regex already rejects non-identifier characters), but making the injection vector explicit in the code is good security practice. - Fail fast: Throwing
InvalidOperationExceptionfromRunTask()is the right call for a security validation —AndroidTaskconverts it to an error automatically, and there's zero chance of continuing with a tainted value. - Testing: The
InvalidCustomJniInitFunctionNametest exercises the full build pipeline end-to-end with both valid and invalid function names, properly usesIgnoreUnsupportedConfiguration, and asserts both build failure and error message content. - Error message: Includes the offending value (
'{name}') — good context for debugging.
💡 Optional follow-up (not blocking): Consider adding a proper XA#### error code with Log.LogCodedError and a Properties.Resources entry in a future PR. This would give users a documented, searchable error code instead of the generic AndroidTask exception wrapper.
CI Status
| Pipeline | Status | Notes |
|---|---|---|
| dotnet-android (Linux) | ✅ Pass | |
| dotnet-android (Mac) | ✅ Pass | |
| dotnet-android (Windows) | ✅ Pass | |
| license/cla | ✅ Pass | |
| Xamarin.Android-PR | ❌ Fail | Infrastructure flake — safe to ignore. The macOS > Tests > MSBuild+Emulator 6 job failed with: "We stopped hearing from agent Azure Pipelines 156." This is an agent connectivity loss, not a code failure. All other failures in that pipeline (APKs 1, Package Tests) are cascading from the lost agent. |
Review generated by android-reviewer from review guidelines.
|
Note that this is classified as a "reliability bug" not security. |
Reject AndroidStaticJniInitFunction items that are not valid C identifiers before they reach the LLVM IR generator. A malicious NuGet package could inject arbitrary LLVM IR directives via newlines or special characters in function names (CWE-74).