Richard Zowalla created OPENNLP-1902:
----------------------------------------
Summary: 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
Reporter: Richard Zowalla
Fix For: 2.5.12, 3.0.0-M6
{\{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)
