Uploaded image for project: 'XWiki Commons'
  1. XWiki Commons
  2. XCOMMONS-3784

extension.repositories.<id>.checksumPolicy is ignored: the value never reaches any download

    XMLWordPrintable

Details

    • Bug
    • Resolution: Unresolved
    • Major
    • None
    • None
    • Extension
    • Unknown

    Description

      llm-agent

      Env

      • source review at xwiki-commons master fd1930b6456 / xwiki-platform master 952e591eb3d; deployed instance XWiki 18.7.0 (xwiki:18.7.0-mariadb-tomcat)
      • no attacker and no XWiki account — reproducible by an administrator with write access to xwiki.properties and a restart
      • the property was added in 10.7RC1 (XCOMMONS-1454, commit be20ce642f, 2018-08-08) and is documented in xwiki.properties.vm:600-601
      • it stopped being honoured on the resolver paths in 15.2-rc-1 (XWIKI-20666, commits bfe607dd33 and 067d23ddb5, both 2023-02-23). Affected: >= 15.2, unchanged through 18.7.0 and master
      • Maven Resolver runtime version is 1.9.27 (maven.resolver.runtime.version, xwiki-commons/pom.xml:278); the precedence below was read from maven-resolver-impl-1.9.27 and maven-resolver-connector-basic-1.9.27 bytecode
      • no CVSS — see h2. Bounds

      Explanation

      XCOMMONS-1627 (Open since 2019-05-06, "Remove error from log 'Could not validate integrity of download from file'", zero comments) and XWIKI-20971 (Open/Minor) both sit on this code path, and both frame it as log noise. The XWIKI-20971 thread is the project deciding twice that the warning is cosmetic — Vincent Massol, 2023-06-08: "This looks like a dup of XCOMMONS-1627 WDYT?"; Thomas Mortagne, 2023-06-08: "This warning is expected (Maven does not generate/download checksum files in the local repository), what is less expected is the fact that they decided to print the stack trace…" — and nobody has asked whether the configuration property that is supposed to control that warning still works. It does not, and the change that broke it is XWIKI-20666, whose commit bfe607dd33 is titled "XWIKI-20666: Upgrade embedded Maven to 3.9.0 and Resolver to 1.9.4 * set a global checksum policy": the session-level override is named in the commit subject itself. (The ticket's own summary says Resolver 1.9.7 — the series went 1.9.4 bfe607dd33 -> 1.9.6 979616d52b -> 1.9.7 1d3f0e21dc.)

      xwiki.properties has documented since 10.7RC1:

      #-# * [Since 10.7RC1] checksumPolicy: what to do when checksum validation fail. Possible values are "fail", "warn"
      #-#   (the default) and "ignore"
      

      The value is plumbed correctly as far as the descriptor — DefaultExtensionManagerConfiguration.setRepositoryProperties() (:228-238) copies every extension.repositories.<id>.* key onto the ExtensionRepositoryDescriptor — and is then read in exactly one place, AetherExtensionRepositoryFactory.java#L108-L118, which applies it to the RemoteRepository:

      // Checksum policy
      String checksumPolicy = repositoryDescriptor.getProperty("checksumPolicy");
      if (StringUtils.isEmpty(checksumPolicy)) {
          checksumPolicy = RepositoryPolicy.CHECKSUM_POLICY_WARN;
      }
      ...
      aetherRepositoryBuilder.setPolicy(new RepositoryPolicy(true, updatePolicy, checksumPolicy));
      

      Nothing that actually validates a checksum ever consults that RemoteRepository policy. There are two independent masks, one per kind of transfer.

      1. The binary artifact download (.jar / .xar). AetherExtensionRepository.java#L360-L376 builds its own ArtifactDownload and hard-codes the policy on it:

      download.setChecksumPolicy(RepositoryPolicy.CHECKSUM_POLICY_WARN);   // :372
      

      BasicRepositoryConnector.get() takes the policy string from ArtifactDownload.getChecksumPolicy() and hands it to ChecksumPolicyProvider.newChecksumPolicy(session, repository, resource, policy). DefaultChecksumPolicyProvider ignores its repository argument entirely and switches only on the string — so the RemoteRepository policy cannot reach this transfer at all, by construction.

      2. Everything the resolver drives (the POM descriptor, maven-metadata.xml, version-range resolution). DefaultArtifactResolver obtains the policy from RemoteRepositoryManager.getPolicy(session, repository, releases, snapshots); DefaultRemoteRepositoryManager.getPolicy delegates to merge(session, p1, p2, globalPolicies) with globalPolicies = true. When the session's own checksum policy is non-null and non-empty, merge returns it and never calls ChecksumPolicyProvider.getEffectiveChecksumPolicy(session, p1, p2):

      155: invokeinterface RepositorySystemSession.getChecksumPolicy:()Ljava/lang/String;
      160: astore 6
      162: iload 4            ; globalPolicies
      164: ifeq  183
      167: aload 6
      169: ifnull 183
      172: aload 6 -> String.isEmpty()
      177: ifne  183
      180: goto  203          ; <-- session value wins; repository policies never read
      183: ... ChecksumPolicyProvider.getEffectiveChecksumPolicy(session, p1.getChecksumPolicy(), p2.getChecksumPolicy())
      

      And XWikiRepositorySystemSession.java#L154-L157 sets that session value unconditionally:

      // Fail when the pom is missing or invalid
      wsession.setArtifactDescriptorPolicy(new SimpleArtifactDescriptorPolicy(false, false));
      // Global checksum and update policy
      wsession.setChecksumPolicy(RepositoryPolicy.CHECKSUM_POLICY_WARN);
      

      Both masks were introduced by the same change, XWIKI-20666, first released in 15.2-rc-1. Before it the session carried no checksum policy, so merge fell through to getEffectiveChecksumPolicy and the per-repository value was honoured on those paths. Mask 1 is older in effect: getFile() has always built its own ArtifactDownload, and before :372 existed it left the policy null, which older resolvers also resolved to warn.

      XCOMMONS-1627's own 2019 log line is not a counter-example to that boundary: it is against maven-metadata.xml, i.e. the resolver path (mask 2), four years before 15.2, and the RepositoryTest harness repository it came from carried no checksumPolicy at all, so merge() legitimately fell through to warn there.

      Net effect: every value of the property collapses to warn. fail is unreachable, and so is ignore — this is not specific to the hardening direction.

      XWiki's own test harness depends on the property: MockitoRepositoryUtils.java:109-117 does descriptor.putProperty("checksumPolicy", "ignore") under the comment // Disable checksum validation. Since 15.2 that has been a no-op. It is harmless there — warn does not fail either — which is exactly why the regression went unnoticed.

      Corroborating counts across xwiki-commons + xwiki-platform (/target/ excluded): CHECKSUM_POLICY_FAIL has 0 occurrences; CHECKSUM_POLICY_WARN has exactly the three sites named above.

      Related: XCOMMONS-1454 (Closed/Fixed, 10.7-rc-1, be20ce642f) delivered the property in the first place — its description's own example is the ignore direction.

      PoC

      Static half — offline, no network, no instance, nothing written:

      set -u
      T=${T:-/path/to/xwiki-platform}     # a checkout containing xwiki-commons/
      
      # the documented property
      grep -n 'checksumPolicy' "$T/xwiki-platform-tools/xwiki-platform-tool-configuration-resources/src/main/resources/xwiki.properties.vm"
      
      # its ONE read site
      grep -rn 'getProperty("checksumPolicy")' --include='*.java' "$T/xwiki-commons" | grep -v '/target/'
      
      # the two masks, and the absence of any FAIL
      grep -rn 'CHECKSUM_POLICY_WARN' --include='*.java' "$T/xwiki-commons" | grep -v '/target/'
      grep -rn 'CHECKSUM_POLICY_FAIL' --include='*.java' "$T" | grep -v '/target/' | wc -l    # expect 0
      
      # XWiki's own harness relying on the property
      grep -n -B2 -A2 'checksumPolicy' \
        "$T/xwiki-commons/xwiki-commons-core/xwiki-commons-extension/xwiki-commons-extension-api/src/test/java/org/xwiki/extension/test/MockitoRepositoryUtils.java"
      
      # when it broke
      git -C "$T/xwiki-commons" log --oneline --date=short --format='%h %ad %s' -S'setChecksumPolicy' --all -- .
      

      Runtime half — not executed here; this is the repro a maintainer can run in a few minutes:

      1. Lay out a one-artifact Maven repository on disk, e.g. /tmp/badrepo/org/xwiki/contrib/foo/1.0/ holding foo-1.0.pom, foo-1.0.jar and their .sha1 / .md5 siblings.
      2. Tamper: append a byte to foo-1.0.jar and leave its .sha1 unchanged, so the recorded digest no longer matches the bytes.
      3. Serve it read-only over plain HTTP: python3 -m http.server 8000 --directory /tmp/badrepo.
      4. In xwiki.properties:
        extension.repositories = badrepo:maven:http://127.0.0.1:8000/
        extension.repositories.badrepo.checksumPolicy = fail
        
      5. Restart, then install org.xwiki.contrib:foo/1.0 from the Extension Manager.

      Expected per the documentation: the download aborts with a ChecksumFailureException and the install fails.
      Predicted from the code: the install succeeds, and the only trace is a single line

      WARN  o.e.a.i.i.WarnChecksumPolicy - Could not validate integrity of download from http://127.0.0.1:8000/org/xwiki/contrib/foo/1.0/foo-1.0.jar
      

      Positive control, required — run it in the same session. Repeat with extension.repositories.badrepo.checksumPolicy = ignore. If the property were honoured, ignore would make DefaultChecksumPolicyProvider.newChecksumPolicy return null and the WarnChecksumPolicy line would disappear. It does not: the identical WARN appears under fail, warn, ignore and no setting at all. That four-way sameness is what distinguishes "the property is ignored" from "the checksum happened to pass" or "the artifact was served from cache".

      A cheaper variant of the same control needs no HTTP server at all: a file:// local Maven repository exercises the same connector, which is why o.e.a.i.i.WarnChecksumPolicy lines against file:///…/.m2/repository/… appear verbatim in XWIKI-20971 despite XWiki's harness asking for ignore.

      Bounds

      • No CVSS, deliberately. This ticket establishes only that a documented configuration property does nothing. There is no attacker and no privilege floor in it; what the missing enforcement is worth is the subject of the companion report on the absent integrity controls.
      • The runtime repro above was NOT run. Everything else — the single read site, both masks, the resolver precedence, the introducing commits and releases — is source- and bytecode-derived at HEAD.
      • ignore is ignored exactly as much as fail is. Any fix should restore both directions, not special-case fail.

      Credit

      Assisted-by: Claude
      Supervised-by: Clément Christiaens (https://github.com/ciaens)

      Attachments

        Activity

          People

            Unassigned Unassigned
            ciaens Clément Christiaens
            Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

              Created:
              Updated: