[
https://issues.apache.org/jira/browse/OPENNLP-1902?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Martin Wiesner updated OPENNLP-1902:
------------------------------------
Component/s: Models
> DownloadUtil does not re-verify cached models against their SHA-512 checksum
> -----------------------------------------------------------------------------
>
> Key: OPENNLP-1902
> URL: https://issues.apache.org/jira/browse/OPENNLP-1902
> Project: OpenNLP
> Issue Type: Improvement
> Components: Models
> Reporter: Richard Zowalla
> Priority: Major
> Fix For: 2.5.12, 3.0.0-M6
>
> Time Spent: 40m
> Remaining Estimate: 0h
>
> {\{DownloadUtil.downloadModel(URL, Class)}} verifies the SHA-512 checksum
> only on the
> path where the model is actually fetched. When the model is already present
> in the
> local download home, the method logs "Model file '{}' already exists. Skipping
> download." and hands the file straight to the model constructor without any
> integrity check:
> if (!Files.exists(localFile)) {
> ...
> Files.copy(in, localFile, StandardCopyOption.REPLACE_EXISTING);
> validateModel(url + ".sha512", localFile); // <-- fresh download only
> } else {
> logger.debug("Model file '{}' already exists. Skipping download.",
> filename);
> }
> The two entry points in this class therefore disagree: \{{existsModel(...)}}
> *does*
> call \{{validateModel(...)}} for an already-present file, while
> \{{downloadModel(...)}}
> — the path that actually loads the model — does not.
> Consequence: once a model is in the download home (~/.opennlp by default,
> overridable
> via the OPENNLP_DOWNLOAD_HOME system property), every subsequent load trusts
> it
> unconditionally. A model that is corrupted on disk, truncated by an
> interrupted
> download, or modified by any process with write access to that directory is
> loaded
> without complaint. This is defense in depth rather than a boundary we
> guarantee —
> per SECURITY.md, model files are trusted input — but the checksum is already
> published and already fetched, so the check is cheap to get right.
> Note the constraint on the fix: \{{validateModel(String, Path)}} downloads the
> ".sha512" file from the CDN each time it is called. Simply invoking it on the
> cache
> path would turn every cached load into a network request and break
> offline/air-gapped
> reuse of the download home, which is a large part of why the cache exists.
> Proposed approach:
> - Persist the ".sha512" file alongside the model on first download.
> - On a cache hit, verify the local model against the locally stored
> checksum, with
> no network access.
> - If the stored checksum is missing (cache populated by an older version),
> either
> re-download the checksum when the network is reachable or fail closed — to
> be
> decided during implementation; a system property to opt out of cache
> verification
> may be warranted for constrained environments.
> - Align \{{existsModel(...)}} and \{{downloadModel(...)}} so both use the
> same
> verification routine.
> - Add tests: valid cached model passes; tampered cached model fails; missing
> checksum sidecar behaves as decided; no network calls on the cache-hit
> path.
> Reported by Jeff Zemerick during review of PR #1198 (OPENNLP-1900). The
> current
> behaviour is documented in SECURITY.md under "Downloaded models"; that note
> should be
> updated when this is fixed.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
