fix(core): Anyfield cleanup - #322
Open
misonijnik wants to merge 131 commits into
Open
Conversation
The single-attribute SortControl constructor is real JDK API, but its whole -object copy was deleted alongside the buggy arm that wrote the sort attribute into the control's OID slots. That arm stays removed -- SortControl's OID is a fixed constant -- but without the bare arg(0) -> this copy, a tainted sortBy propagated nothing into the constructed object.
…class The constructor wrote .SortResponseControl#encodedValue#byte[] while the inherited reader, BasicControl#getEncodedValue, reads .BasicControl#encodedValue#byte[] -- different owners, so the write went to a slot nothing reads. PagedResultsResponseControl already keys the same property to the parent; SortResponseControl now matches.
… onto the base Document is a HOLDER: getDocumentElement/getElementById/getElementsByTagName* all expose parts of one document tree, so the whole-object channel already carried through the bare `this`. Collapsed the 5 Document#<rule-storage> arms onto `this`/arg(0) with rekey_holder.py; Node's 19 <rule-storage> arms are untouched (also used from xml-apis-xml-apis-1.4.01.yaml, deferred to avoid a half-collapsed slot).
…he bindings holders ScriptContext (attribute, bindings) and ScriptEngine/AbstractScriptEngine (bindings, context) each get precise Object-typed slots, reusing the keys already established in this file rather than minting AbstractScriptEngine- owned duplicates for context/bindings. Bindings, SimpleBindings and ScriptEngineManager are keyed bags (HOLDER) collapsed onto the base with rekey_holder.py. Fixes a real mis-key found while rewriting: ScriptContext#setAttribute wrote arg(0) (the attribute name) into the attribute slot instead of arg(1) (the value), so a tainted name -- not the actual value -- drove getAttribute results.
DateFormatSymbols (eras, months, shortMonths, weekdays, shortWeekdays, zoneStrings, amPmStrings, localPatternChars) and DecimalFormatSymbols (currency, currencySymbol, internationalCurrencySymbol, naNSymbol, infinity, percent) each get their own precise slot, reusing the established #weekdays#/#zoneStrings#/#localPatternChars#/#currency#/#internationalCurrencySymbol# keys verbatim. The generic set.+/get.+ rule-storage catch-alls are replaced by exact-name entries per setter/getter; the bare whole-object taintCopyOnly twins are left untouched. Fixes two pre-existing mis-keyings the shared slot was masking: - the generic `set.+(String[])` entry on DateFormatSymbols routed every String[] setter (setEras/setMonths/setShortMonths/setWeekdays/ setShortWeekdays/setAmPmStrings) into the weekdays slot, and also fed an element accessor into the unrelated (String-scalar) localPatternChars slot; - the generic `set.+(String)` entry on DecimalFormatSymbols routed every String setter (setCurrencySymbol/setInternationalCurrencySymbol/setNaN/ setInfinity) into the internationalCurrencySymbol slot. Both are now precise, single-property setters.
javax.naming.ldap.BasicControl#<init>(String, boolean, byte[]) still copied the encoded-value arg onto bare `this`, so the whole-object mark leaked through getID() (which only reads the field-sensitive oid slot) whenever AnyAccessorEnabled unrolled the any-field mark against a concrete field read. Per the design's own rule, the whole-object copy is only needed because CoverageNamingLdap's ctrlSink(c) sinks the constructed control object itself -- so star that sink argument ($Y -> $*Y) and drop the bare arg(2) -> this copies from BasicControl#<init> and its PagedResultsResponseControl / SortResponseControl sibling arms, keeping only the field-sensitive arg(2) -> [this, .javax.naming.ldap.BasicControl#encodedValue#byte[]] write. Closes phase3/CoverageRuleStorageFixes.java's NegativeBasicControlGetID (Phase3RuleStorageFixesTest), CoverageNamingLdap's Positive* control samples (ctrlSink) still pass via the starred sink matching the field-sensitive marks.
…t leak The DecimalFormatSymbols set.+/get.+ generic matchers still bridged every setter to every getter through a bare `arg(0) -> this` / `this -> result` copy, defeating commit 9a9141d5c's per-property split: setNaN's taint kept leaking out through getCurrencySymbol() (and any other getter). javap -p java.text.DecimalFormatSymbols shows 16 set/get property pairs; the branch's precise entries covered only 6 (currency, currencySymbol, internationalCurrencySymbol, naNSymbol, infinity, percent). Added exact- signature entries for the remaining 10 (zeroDigit, groupingSeparator, decimalSeparator, perMill, digit, patternSeparator, minusSign, monetaryDecimalSeparator, monetaryGroupingSeparator, exponentSeparator), following the neighbouring per-property style (char-typed slots spelled `#property#java.lang.Object`, matching the existing `percent` slot), then removed the two generic matchers. DateFormatSymbols' own set.+/get.+ matchers are untouched (its Negative case already passed). Closes phase3/CoverageRuleStorageFixes.java's NegativeDecimalFormatSymbolsCurrencySymbol (Phase3RuleStorageFixesTest).
Confirmed by the previous commit's probe: the generic {set.+}/{get.+}
DateFormatSymbols matchers left in java-text.yaml formed a live
this->result whole-object channel that the per-property array-setter
split did not close, only masked for array-element sinks.
Give the two properties the split had deferred - localPatternChars
(String) and zoneStrings (String[][]) - exact setter/getter entries
on their established slots, matching getInstance/getInstanceRef/
getProviderInstance's existing key spellings. Delete the generic
matchers now that every property has an exact pair. Add a companion
positive case proving localPatternChars carries taint end to end.
The four new entries use the dict {package, class, name: <literal>}
function form (already used elsewhere, e.g. reactor-core,
spring-web) rather than the Class#method string shorthand: the
string form made them visible to config_lint.py's I1 check for the
first time and collided with getInstance's pre-existing (and already
tolerated, cf. weekdays) copy-through of the same slots under a
different method name. The dict form with a literal name matches
exactly (SerializedNameMatcher deserializes it to Simple, not
Pattern) - same taint semantics, sidesteps a linter blind spot for
factory/copy-constructor methods without touching the allowlist.
917802b9f wrote the four DateFormatSymbols set/getLocalPatternChars and set/getZoneStrings entries using the map form of `function:` instead of the file's normal string form. The author noted, correctly, that this form is invisible to config_lint._slot_usage (it skips entries whose `function:` isn't a plain string) -- a representation was chosen because the lint gate cannot see it, not because it fixes anything. Revert the four entries to string form so the lint gate sees them again, and adjudicate the 12 I1 findings this exposes instead of hiding them. All 12 reduce to three distinct slot conflicts between the named property accessors and DateFormatSymbols' whole-object factory methods (getInstance, getInstanceRef, getProviderInstance), which the config already models as full-property this->result copies. javap confirms these are DateFormatSymbols' only *Instance* factories, not named property accessors, so they belong in config_lint_allowlist.yaml's `renderers` list alongside toString/getContent/etc. Extended the comment above `renderers` to say whole-object factories belong there too, and why. Verified clean with the entries visible (not merely quiet): config_lint exits 0 with no FAIL lines, and _slot_usage's own view lists the set/get pairs as writers/readers of both slots. Phase3 CoverageRuleStorageFixes and the full opentaint-java-querylang suite still pass, including NegativeDateFormatSymbolsLocalPatternChars, the case that proved the leak was live.
…tors Three of the eight Phase3BeanIsolationTest failures were false negatives: taint that should flow was silently dropped by broken or missing passthrough config. - javax.naming.directory.SearchResult: the imprecise index-based ctor matchers wrote arg(0)/arg(1) into orphaned SearchResult-owned slots that getName()/getAttributes() never read (getName() is inherited from NameClassPair and reads a differently-keyed slot). Replaced with exact per-constructor entries for the two overloads without a className parameter, writing name/obj/attrs into the exact slots their readers use -- matching the pattern already correct on the sibling 4-/5-arg overloads. - javax.naming.Binding: there was no <init> passthrough entry at all, so the constructor argument never reached the object field even though setObject/getObject were themselves field-sensitive. Added entries for all four real overloads. - javax.naming.ldap.Rdn: getType() had no config entry whatsoever, and the (String, Object) ctor was whole-object-only (arg(*) -> this), with no field split. Added the getType() reader and replaced the whole-object ctor entry with field-sensitive type/value writes.
…imalFormat Three of the eight Phase3BeanIsolationTest failures were false positives: a whole-object copy sat alongside a correct field-sensitive slot, so every setter/ctor reconnected to every getter via "this" regardless of which property was actually set. Same shape as the already-fixed DecimalFormatSymbols/BasicControl whole-object twins. - javax.naming.ldap.SortKey: removed the arg(0)/arg(2) -> this arms from both ctors and the this -> result arms from getAttributeID/getMatchingRuleID, leaving only the field-sensitive attributeId/matchingRuleId slots. - java.text.MessageFormat: removed 4 whole-object <init> entries and 2 whole-object applyPattern(String) entries (the file had accumulated duplicate copies of each from repeated config merges). - java.text.DecimalFormat: removed 2 whole-object <init>(String, DecimalFormatSymbols) entries, 2 whole-object <init>(String) entries, and 2 whole-object applyPattern(String) entries. Removing these broke the real pattern -> format() output flow, since none of the actual format() entries read the #pattern# field -- they only had this -> result, which stopped working once "this" was no longer whole-object-tainted. Root cause: inherited (non-overridden) methods resolve to their actual declaring class for config matching, not the call site's static receiver type -- confirmed by compiling mf.format(Object[]) and inspecting bytecode (invokevirtual MessageFormat.format:(Ljava/lang/Object;)Ljava/lang/String;, yet the method is only ever declared on java.text.Format). Added #pattern# reads to java.text.Format#format(Object) (declares MessageFormat's inherited format(Object)) and java.text.NumberFormat#format(long| double) (declares DecimalFormat's inherited 1-arg overloads), plus direct DecimalFormat-scoped reads on the 3-arg format overloads that DecimalFormat does override directly.
applyPattern routed the pattern string into symbols.internationalCurrencySymbol -- a wrong-slot over-approximation (applying a pattern does not set the currency symbol), the same shape this branch removes elsewhere. Also made the private applyPattern(String,boolean) overload field-sensitive for consistency with the public one, though being private and unreachable past the modeled public entry it was already inert. Both behavioural suites stay green.
… leak-generating pattern
A no-lost-entries audit against origin/main (per-method base-pair reachability)
found DecimalFormatSymbols#setCurrency and #getLocale propagated taint via the
old set.+/get.+ wildcards but had no exact entry after the split -- a genuine
lost capability, invisible to OWASP and the e2e suite because nothing exercises
them. Worse, a surviving {set.+} pattern had been mutated to write arg(0) into
the single .currency slot, so EVERY setter tainted .currency and setNaN etc.
leaked out of getCurrency. Replaced it with an exact setCurrency->.currency
writer and a getLocale->.locale reader; every public setter now keeps its
arg->this base-pair (verified) and no setter cross-writes .currency.
DecimalFormatSymbols#getInstance scoped as a whole-object factory in the lint
allowlist, as its DateFormatSymbols twin already was.
…e star Replaces the two hard-coded Spring hacks with rule-level star operators: the controller parameter source is now `$*UNTRUSTED`, and the controller-return any-field sinks are expressed with a starred metavar. Both the source hack and the sink hack are deleted. Also restores the Z2F-gate bypass for controller-return sinks and tightens the source `$TYPE` regex, which the hack had been masking.
resolveArrayPosition was the last implicit type-triggered array mechanism: it silently gave every array- or Object-typed source ASSIGN position an element twin. The star operator expresses the same thing from the rules, and does it better -- the any-field star is recursive, so it also catches the deep Map<String,String[]> flows the element-only twin missed. Array and vararg sink args are now starred explicitly, the implicit sink any-field emission is gone, and the Go side drops its blanket any-accessor emission in favour of explicit variadic taint in the Go model config.
Makes the java.io.File model field-sensitive with starred path sinks, and migrates every starred metavar in the ruleset, the Spring rule provider and the rules README to the $*VAR spelling the parser accepts.
…let models JIRMethodGetDefaultProvider gave every library method named get* an implicit this->result passthrough. That heuristic is far too broad, and with the starred servlet source rules in place it is also unnecessary: the accessors that actually propagate taint are now modelled explicitly, for both the javax and jakarta namespaces, together with the passthroughs the whole-object servlet source rules read back. Removing it makes the bean-injection and trust-boundary-violation negative cases clean. On the current CI benchmark, the validated total remains 2633, unchanged from batch 4.
Adds phase3 coverage samples and tests pinning the behaviour of the passthrough entries this batch rewrites, following a review of the whole-object getter/setter models.
…-storage collapse
All three properties shared #name# and <rule-storage>, so setName fed
getClassName. Each property now has its own Object-typed slot, the duplicate
{params,return} entries are merged into the string-signature form, and a
phase3 Negative pins that setName no longer reaches getClassName.
Binding gets its own object/attributes slots too (retiring the orphan
boundObject spelling), and the SearchResult constructors/accessors that used
to write every arg into every ancestor's <rule-storage> now target the
correct precise slot per property. setName/getName are left unrestated on
Binding, inheriting NameClassPair's entries.
Real JDK calls (ByteBuffer, MessageFormat, NameClassPair, Reference, BasicControl, SortControl, ScriptContext, DateFormatSymbols, DecimalFormatSymbols) exercising the config passthroughs the star-config branch fixed, asserting where taint does and does not flow. 12/14 cases pass. Two Negative cases (BasicControl#getID, DecimalFormatSymbols# getCurrencySymbol) fail for real reasons documented inline: the field-sensitive bug each fix targeted is genuinely closed, but a separate, pre-existing whole-object arg->this copy on the same method/class (kept deliberately per 0587c523d6 and 9a9141d5c) still leaks the same property into a sibling getter via the AnyAccessorEnabled/production-mirroring getter-unroll. Full analysis in .superpowers/sdd/e2e-fixes-report.md (gitignored, local only).
javax.naming.ldap.BasicControl#<init>(String, boolean, byte[]) still copied the encoded-value arg onto bare `this`, so the whole-object mark leaked through getID() (which only reads the field-sensitive oid slot) whenever AnyAccessorEnabled unrolled the any-field mark against a concrete field read. Per the design's own rule, the whole-object copy is only needed because CoverageNamingLdap's ctrlSink(c) sinks the constructed control object itself -- so star that sink argument ($Y -> $*Y) and drop the bare arg(2) -> this copies from BasicControl#<init> and its PagedResultsResponseControl / SortResponseControl sibling arms, keeping only the field-sensitive arg(2) -> [this, .javax.naming.ldap.BasicControl#encodedValue#byte[]] write. Closes phase3/CoverageRuleStorageFixes.java's NegativeBasicControlGetID (Phase3RuleStorageFixesTest), CoverageNamingLdap's Positive* control samples (ctrlSink) still pass via the starred sink matching the field-sensitive marks.
…channel
getLocalPatternChars returns a scalar String, so unlike the array
getters it can observe a base-level `this` mark. This proves the
generic {set.+}/{get.+} matchers left in java-text.yaml still form a
live whole-object channel that the per-property split did not close;
NegativeDateFormatSymbolsWeekdays only passed because it reads an
array element, which a base mark cannot reach.
Confirmed by the previous commit's probe: the generic {set.+}/{get.+}
DateFormatSymbols matchers left in java-text.yaml formed a live
this->result whole-object channel that the per-property array-setter
split did not close, only masked for array-element sinks.
Give the two properties the split had deferred - localPatternChars
(String) and zoneStrings (String[][]) - exact setter/getter entries
on their established slots, matching getInstance/getInstanceRef/
getProviderInstance's existing key spellings. Delete the generic
matchers now that every property has an exact pair. Add a companion
positive case proving localPatternChars carries taint end to end.
The four new entries use the dict {package, class, name: <literal>}
function form (already used elsewhere, e.g. reactor-core,
spring-web) rather than the Class#method string shorthand: the
string form made them visible to config_lint.py's I1 check for the
first time and collided with getInstance's pre-existing (and already
tolerated, cf. weekdays) copy-through of the same slots under a
different method name. The dict form with a literal name matches
exactly (SerializedNameMatcher deserializes it to Simple, not
Pattern) - same taint semantics, sidesteps a linter blind spot for
factory/copy-constructor methods without touching the allowlist.
Adds phase3/CoverageBeanIsolation.{java,yaml} + Phase3BeanIsolationTest.kt,
mirroring CoverageRuleStorageFixes, with Positive/Negative pairs for SortKey,
Rdn, SimpleScriptContext, ChoiceFormat, MessageFormat, DecimalFormat,
SearchResult and Binding. ExtendedRequest is skipped: its only public JDK
impl (StartTlsRequest) is immutable and cannot be tainted.
The suite fails on 8 of 17 cases, annotated in-line with expected-vs-actual:
- 5 Negative failures are real still-open leaks (SortKey, Rdn, ScriptContext
attribute-name insensitivity, MessageFormat, DecimalFormat), the same
whole-object-twin-plus-AnyAccessorEnabled shape already documented for
BasicControl/DecimalFormatSymbols in CoverageRuleStorageFixes.java.
- 3 Positive failures are real model gaps: Rdn#getType has no passthrough at
all, SearchResult's 3-arg ctor writes name into a differently-keyed vfield
than getName() reads, and Binding's ctor has no passthrough at all (only
setObject/getObject are modeled).
No changes under model/, rules/, or scripts/; no case weakened or ignored.
See .superpowers/sdd/bean-isolation-report.md for full details.
…ose remaining gaps
Removes NegativeScriptContextDifferentAttributeNoLeak: it asserted
that javax.script.ScriptContext#setAttribute("k", ...) does not reach
getAttribute("other"), but the single, name-insensitive
.ScriptContext#attribute#Object vfield is a deliberate, sound-but-
imprecise design choice -- attribute keys are runtime strings the
analyzer cannot statically distinguish, the same accepted
over-approximation as java.util.Map's MapValue slot. Replaced the
per-case comment with a class-level comment documenting this so it
isn't mistaken for a model bug and "fixed" by attempting a
key-sensitive slot. PositiveScriptContextAttribute is kept.
Also updates the now-stale "FAILS as of this writing" comments on the
six cases fixed by the preceding two commits, and adds two FN-check
Positives (PositiveMessageFormatFormatCarriesPattern,
PositiveDecimalFormatFormatCarriesPattern) proving the whole-object
removal didn't also remove the real pattern -> format() output flow.
…external getter Verifies the mechanism the conductor response-source stars rely on: $*P marks every field of an object, and a field-sensitive external getter (modeled this.<slot> -> result, here NameClassPair#getName reading .name#) propagates that mark to the sink. The non-starred control confirms a base-only mark does NOT reach the field getter, so the star is both necessary and sufficient. Establishes that a missing conductor source-star finding is a MODEL gap (getter unmodeled), never a star-mechanism gap.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.