[
https://issues.apache.org/jira/browse/DRILL-8554?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18123545#comment-18123545
]
ASF GitHub Bot commented on DRILL-8554:
---------------------------------------
pjfanning commented on PR #3080:
URL: https://github.com/apache/drill/pull/3080#issuecomment-6003118624
*This review was generated by Claude AI (Anthropic), posted on behalf of
@pjfanning.*
The direction is right: using a rejecting `LinkResolver` is the correct
hook, opening read-only is a good extra, and `equals`/`hashCode`/`toString` are
all updated. There are a few issues, the first one significant.
### 1. Any query user can switch the protection off (main concern)
The README documents enabling this per query, and that works because
table-function parameters override the format config:
```sql
FROM table(dfs.`x.accdb` (type=>'msaccess', tableName=>'Linked',
allowLinkedDatabases=>true))
```
`THREAT_MODEL.md` treats plugin config as admin-only (§3/§6, `OUT-OF-MODEL:
trusted-input`), but this flag can be set by any query user. Someone who can
upload an `.accdb` to a writable workspace and query it can turn the flag on
themselves. The Drillbit then opens any local or UNC path as its service
account, which also exposes it to NTLM hash capture over SMB. That is the
attack this PR is meant to stop.
Suggestions:
- Make this an admin-only setting, such as a system/boot option or a
plugin-level setting that table functions can't override. At minimum, have the
reader ignore the flag when it comes from a table function.
- Remove the "It can also be enabled per query" section from the README.
### 2. Listing tables now fails for any file that has a linked table
With no `tableName`, the reader iterates `db.getTableNames()` and calls
`db.getTable(name)` for each one in `processMetadataRow`. If Jackcess includes
linked tables in `getTableNames()` (I believe it does), the rejecting resolver
throws and the whole metadata query fails. Users can't even discover what is in
the file. It would be better to skip linked tables, or return them with null
counts, instead of failing. For example, check
`db.getTableMetaData(name).isLinked()` before calling `getTable`. A test of the
metadata query on a file with a linked table would confirm the behaviour.
### 3. Misleading error message
`buildSchemaFromTable` wraps the error as `"Table Linked not found. Refusing
to open linked database…"`. The table does exist, so "not found" is wrong. A
clear `UserException` that says linked tables are disabled, and names the
setting that enables them, would be better.
### 4. Test robustness
- `testLinkedTableResolvedWhenAllowed` links to
`//evil.example.com/share/secret.mdb`. On a Windows build agent with the flag
on, Jackcess would attempt real SMB/DNS. A path under `dirTestWatcher` that
doesn't exist would avoid that.
- The test checks for Jackcess's `"given file does not exist"` text, which
may change between versions. A stronger test would create a real target
database with a row in it, link to it, and assert the row comes back when the
flag is on.
- Nothing tests the plugin-level setting on its own, without the table
function.
### 5. Minor points
- The public `@JsonCreator` constructor changes arity. There are no in-repo
callers, so this is fine for a contrib plugin.
- Unrelated, but nearby: in `processMetadataRow`, `updated_date` reads
`table.getCreatedDate()`. It should probably be `getUpdatedDate()`.
- The new `LinkResolver` import is out of alphabetical order.
- Release notes should say that linked tables are now off by default, since
that changes behaviour for existing users.
**Suggest changes before merge:** #1 lets a non-admin user bypass the
protection, and #2 is a usability regression worth fixing in the same PR.
> Update Jackcess and Fix Linked Table Error
> ------------------------------------------
>
> Key: DRILL-8554
> URL: https://issues.apache.org/jira/browse/DRILL-8554
> Project: Apache Drill
> Issue Type: Bug
> Components: Format - MS Access
> Affects Versions: 1.22.0
> Reporter: Charles Givre
> Assignee: Charles Givre
> Priority: Critical
> Fix For: 1.23.0
>
>
> This PR bumps Jackcess to the latest version, 4.0.11. It also adds a new
> configuration option to enable/disable linked tables in MS Access files.
> The option defaults to `false`.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
