diff --git a/contrib/format-access/README.md b/contrib/format-access/README.md index 0c3035117aa..230eda91dff 100644 --- a/contrib/format-access/README.md +++ b/contrib/format-access/README.md @@ -11,6 +11,24 @@ Simply add the following to any Drill file system configuration. Typically, MS } ``` +### Linked Tables +An Access file can define tables that live in another database file, referenced by an absolute local path or a UNC +share embedded in the file itself. Drill refuses to follow those references by default: reading an untrusted file +would otherwise make the Drillbit open a path of the file author's choosing, as the Drillbit service account. + +**Note:** Before Drill 1.23, linked tables were followed automatically. Queries against a linked table now fail with a +permission error unless an administrator enables them. Listing the tables in a file (a query without `tableName`) +still works: linked tables are shown with null dates and counts. + +If you trust every file that can be queried and need linked tables to work, an administrator can enable them by +setting the following boot option in `drill-override.conf` on every Drillbit: + +``` +drill.exec.storage.msaccess.allow_linked_databases: true +``` + +This is deliberately not a format config or table-function option, so that a query user cannot turn it on. + ## Schemas Drill will discover the schema automatically from the Access file. The plugin does support schema provisioning for consistency, but is not recommended. diff --git a/contrib/format-access/pom.xml b/contrib/format-access/pom.xml index 288bc9d92d9..82621466717 100644 --- a/contrib/format-access/pom.xml +++ b/contrib/format-access/pom.xml @@ -39,7 +39,7 @@ com.healthmarketscience.jackcess jackcess - 4.0.8 + 4.0.11 commons-logging diff --git a/contrib/format-access/src/main/java/org/apache/drill/exec/store/msaccess/MSAccessBatchReader.java b/contrib/format-access/src/main/java/org/apache/drill/exec/store/msaccess/MSAccessBatchReader.java index 735c2cc71fc..070f7931852 100644 --- a/contrib/format-access/src/main/java/org/apache/drill/exec/store/msaccess/MSAccessBatchReader.java +++ b/contrib/format-access/src/main/java/org/apache/drill/exec/store/msaccess/MSAccessBatchReader.java @@ -24,6 +24,8 @@ import com.healthmarketscience.jackcess.DatabaseBuilder; import com.healthmarketscience.jackcess.Row; import com.healthmarketscience.jackcess.Table; +import com.healthmarketscience.jackcess.TableMetaData; +import com.healthmarketscience.jackcess.util.LinkResolver; import org.apache.commons.lang3.StringUtils; import org.apache.drill.common.AutoCloseables; import org.apache.drill.common.exceptions.CustomErrorContext; @@ -58,6 +60,16 @@ public class MSAccessBatchReader implements ManagedReader { private static final Logger logger = LoggerFactory.getLogger(MSAccessBatchReader.class); + /** + * An Access file can name another database (a local path or a UNC share) as the source of a + * linked table. Jackcess's default resolver would open it as the Drillbit service account, so + * refuse instead unless {@link MSAccessFormatPlugin#ALLOW_LINKED_DATABASES} is set. Linked tables + * are normally caught earlier with a clearer error; this is the backstop. + */ + private static final LinkResolver REJECT_LINKED_DATABASES = (linkerDb, linkeeFileName) -> { + throw new IOException("Refusing to open linked database referenced by this MS Access file: " + linkeeFileName); + }; + private final FileDescrip file; private final CustomErrorContext errorContext; private final RowSetLoader rowWriter; @@ -65,6 +77,7 @@ public class MSAccessBatchReader implements ManagedReader { private final List columnList; private final MSAccessFormatConfig config; private final boolean metadataOnly; + private final boolean allowLinkedDatabases; private File tempFile; private Set tableList; private Iterator rowIterator; @@ -73,8 +86,10 @@ public class MSAccessBatchReader implements ManagedReader { private Table table; private Database db; - public MSAccessBatchReader(FileSchemaNegotiator negotiator, File tempDir, MSAccessFormatConfig config) { + public MSAccessBatchReader(FileSchemaNegotiator negotiator, File tempDir, MSAccessFormatConfig config, + boolean allowLinkedDatabases) { this.tempDir = tempDir; + this.allowLinkedDatabases = allowLinkedDatabases; this.columnList = new ArrayList<>(); this.config = config; this.file = negotiator.file(); @@ -123,10 +138,11 @@ private void buildSchema(FileSchemaNegotiator negotiator) { private TupleMetadata buildMetadataSchema(SchemaBuilder builder) { // Adds the table name builder.add("table", MinorType.VARCHAR); - builder.add("created_date", MinorType.TIMESTAMP); - builder.add("updated_date", MinorType.TIMESTAMP); - builder.add("row_count", MinorType.INT); - builder.add("col_count", MinorType.INT); + // Nullable because linked tables are listed without being opened, so these are unknown. + builder.addNullable("created_date", MinorType.TIMESTAMP); + builder.addNullable("updated_date", MinorType.TIMESTAMP); + builder.addNullable("row_count", MinorType.INT); + builder.addNullable("col_count", MinorType.INT); builder.addArray("columns", MinorType.VARCHAR); return builder.buildSchema(); @@ -134,11 +150,28 @@ private TupleMetadata buildMetadataSchema(SchemaBuilder builder) { private TupleMetadata buildSchemaFromTable(SchemaBuilder builder, String tableName) { try { + TableMetaData metaData = db.getTableMetaData(tableName); + if (metaData == null) { + deleteTempFile(); + throw UserException.validationError() + .message("Table " + tableName + " not found.") + .addContext(errorContext) + .build(logger); + } + if (isBlockedLink(metaData)) { + deleteTempFile(); + throw UserException.permissionError() + .message("Table " + tableName + " is a linked table that refers to another database (" + + metaData.getLinkedDbName() + "). Reading linked tables is disabled. An administrator can " + + "enable it by setting " + MSAccessFormatPlugin.ALLOW_LINKED_DATABASES + " to true in drill-override.conf.") + .addContext(errorContext) + .build(logger); + } table = db.getTable(tableName); } catch (IOException e) { deleteTempFile(); throw UserException.dataReadError(e) - .message("Table " + config.getTableName() + " not found. " + e.getMessage()) + .message("Error opening table " + tableName + ": " + e.getMessage()) .addContext(errorContext) .build(logger); } @@ -247,7 +280,12 @@ public boolean next() { private void openFile() { try { fsStream = file.fileSystem().openPossiblyCompressedStream(file.split().getPath()); - db = DatabaseBuilder.open(convertInputStreamToFile(fsStream)); + db = new DatabaseBuilder(convertInputStreamToFile(fsStream)) + .setReadOnly(true) + .open(); + if (!allowLinkedDatabases) { + db.setLinkResolver(REJECT_LINKED_DATABASES); + } tableList = db.getTableNames(); } catch (IOException e) { deleteTempFile(); @@ -258,9 +296,21 @@ private void openFile() { } } + private boolean isBlockedLink(TableMetaData metaData) { + return metaData.isLinked() && !allowLinkedDatabases; + } + private void processMetadataRow(String tableName) throws IOException { Table table; try { + TableMetaData metaData = db.getTableMetaData(tableName); + if (metaData != null && isBlockedLink(metaData)) { + // List the linked table without opening it, so the rest of the file can still be explored. + rowWriter.start(); + rowWriter.scalar("table").setString(tableName); + rowWriter.save(); + return; + } table = db.getTable(tableName); } catch (IOException e) { deleteTempFile(); @@ -274,7 +324,7 @@ private void processMetadataRow(String tableName) throws IOException { rowWriter.scalar("table").setString(tableName); LocalDateTime createdDate = table.getCreatedDate(); rowWriter.scalar("created_date").setTimestamp(createdDate.toInstant(ZoneOffset.UTC)); - LocalDateTime updatedDate = table.getCreatedDate(); + LocalDateTime updatedDate = table.getUpdatedDate(); rowWriter.scalar("updated_date").setTimestamp(updatedDate.toInstant(ZoneOffset.UTC)); rowWriter.scalar("row_count").setInt(table.getRowCount()); rowWriter.scalar("col_count").setInt(table.getColumnCount()); diff --git a/contrib/format-access/src/main/java/org/apache/drill/exec/store/msaccess/MSAccessFormatPlugin.java b/contrib/format-access/src/main/java/org/apache/drill/exec/store/msaccess/MSAccessFormatPlugin.java index 16508acfa42..38d9f1e7502 100644 --- a/contrib/format-access/src/main/java/org/apache/drill/exec/store/msaccess/MSAccessFormatPlugin.java +++ b/contrib/format-access/src/main/java/org/apache/drill/exec/store/msaccess/MSAccessFormatPlugin.java @@ -39,20 +39,30 @@ public class MSAccessFormatPlugin extends EasyFormatPlugin { protected static final String DEFAULT_NAME = "msaccess"; + + /** + * Boot option that lets the reader follow linked tables to other database files. It is + * deliberately not part of {@link MSAccessFormatConfig}: every format config field can be + * overridden from a table function, which would let any query user turn the protection off. + */ + public static final String ALLOW_LINKED_DATABASES = "drill.exec.storage.msaccess.allow_linked_databases"; private final DrillbitContext context; private static class MSAccessReaderFactory extends FileReaderFactory { private final File tempDir; private final MSAccessFormatConfig config; - public MSAccessReaderFactory(File tempDir, MSAccessFormatConfig config) { + private final boolean allowLinkedDatabases; + + public MSAccessReaderFactory(File tempDir, MSAccessFormatConfig config, boolean allowLinkedDatabases) { this.tempDir = tempDir; this.config = config; + this.allowLinkedDatabases = allowLinkedDatabases; } @Override public ManagedReader newReader(FileSchemaNegotiator negotiator) { - return new MSAccessBatchReader(negotiator, tempDir, config); + return new MSAccessBatchReader(negotiator, tempDir, config, allowLinkedDatabases); } } @@ -82,7 +92,12 @@ private static EasyFormatConfig easyConfig(Configuration fsConf, MSAccessFormatC @Override protected void configureScan(FileScanLifecycleBuilder builder, EasySubScan scan) { builder.nullType(Types.optional(TypeProtos.MinorType.VARCHAR)); - builder.readerFactory(new MSAccessReaderFactory(getTmpDir(), formatConfig)); + builder.readerFactory(new MSAccessReaderFactory(getTmpDir(), formatConfig, allowLinkedDatabases())); + } + + private boolean allowLinkedDatabases() { + DrillConfig config = context.getConfig(); + return config.hasPath(ALLOW_LINKED_DATABASES) && config.getBoolean(ALLOW_LINKED_DATABASES); } /** diff --git a/contrib/format-access/src/main/resources/drill-module.conf b/contrib/format-access/src/main/resources/drill-module.conf index 7004e22a8bb..d7216e75654 100644 --- a/contrib/format-access/src/main/resources/drill-module.conf +++ b/contrib/format-access/src/main/resources/drill-module.conf @@ -22,3 +22,11 @@ drill.classpath.scanning: { packages += "org.apache.drill.exec.store.msaccess" } + +drill.exec.storage.msaccess: { + # An Access file can define linked tables that point at another database file by an absolute + # local path or a UNC share. Following them makes the Drillbit open a path chosen by the file's + # author, as the Drillbit service account, so this is off by default. It is a boot option so + # that it can only be enabled by an administrator, not by a query. + allow_linked_databases: false +} diff --git a/contrib/format-access/src/test/java/org/apache/drill/exec/store/msaccess/TestMSAccessLinkedTables.java b/contrib/format-access/src/test/java/org/apache/drill/exec/store/msaccess/TestMSAccessLinkedTables.java new file mode 100644 index 00000000000..4d4ec581f8d --- /dev/null +++ b/contrib/format-access/src/test/java/org/apache/drill/exec/store/msaccess/TestMSAccessLinkedTables.java @@ -0,0 +1,83 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.drill.exec.store.msaccess; + +import com.healthmarketscience.jackcess.ColumnBuilder; +import com.healthmarketscience.jackcess.DataType; +import com.healthmarketscience.jackcess.Database; +import com.healthmarketscience.jackcess.DatabaseBuilder; +import com.healthmarketscience.jackcess.Table; +import com.healthmarketscience.jackcess.TableBuilder; +import org.apache.drill.categories.RowSetTest; +import org.apache.drill.common.types.TypeProtos.MinorType; +import org.apache.drill.exec.physical.rowSet.RowSet; +import org.apache.drill.exec.physical.rowSet.RowSetBuilder; +import org.apache.drill.exec.record.metadata.SchemaBuilder; +import org.apache.drill.exec.record.metadata.TupleMetadata; +import org.apache.drill.test.ClusterFixture; +import org.apache.drill.test.ClusterTest; +import org.apache.drill.test.rowSet.RowSetComparison; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +import java.io.File; + +/** + * Linked tables with {@link MSAccessFormatPlugin#ALLOW_LINKED_DATABASES} enabled by the administrator. + * The default (disabled) behaviour is covered in {@link TestMSAccessReader}. + */ +@Category(RowSetTest.class) +public class TestMSAccessLinkedTables extends ClusterTest { + + @BeforeClass + public static void setup() throws Exception { + ClusterTest.startCluster(ClusterFixture.builder(dirTestWatcher) + .configProperty(MSAccessFormatPlugin.ALLOW_LINKED_DATABASES, true)); + } + + @Test + public void testLinkedTableResolvedWhenAllowed() throws Exception { + File target = new File(dirTestWatcher.getRootDir(), "linked_target.mdb"); + try (Database db = DatabaseBuilder.create(Database.FileFormat.V2003, target)) { + Table table = new TableBuilder("Table1") + .addColumn(new ColumnBuilder("name", DataType.TEXT)) + .toTable(db); + table.addRow("from the linked database"); + } + + File linker = new File(dirTestWatcher.getRootDir(), "linker.mdb"); + try (Database db = DatabaseBuilder.create(Database.FileFormat.V2003, linker)) { + db.createLinkedTable("Linked", target.getAbsolutePath(), "Table1"); + } + + String sql = "SELECT * FROM table(dfs.`" + linker.getName() + "` (type=> 'msaccess', tableName => 'Linked'))"; + RowSet results = client.queryBuilder().sql(sql).rowSet(); + + TupleMetadata expectedSchema = new SchemaBuilder() + .addNullable("name", MinorType.VARCHAR) + .buildSchema(); + + RowSet expected = new RowSetBuilder(client.allocator(), expectedSchema) + .addRow("from the linked database") + .build(); + + new RowSetComparison(expected).verifyAndClearAll(results); + } +} diff --git a/contrib/format-access/src/test/java/org/apache/drill/exec/store/msaccess/TestMSAccessReader.java b/contrib/format-access/src/test/java/org/apache/drill/exec/store/msaccess/TestMSAccessReader.java index e05e57f814f..c966d1f60eb 100644 --- a/contrib/format-access/src/test/java/org/apache/drill/exec/store/msaccess/TestMSAccessReader.java +++ b/contrib/format-access/src/test/java/org/apache/drill/exec/store/msaccess/TestMSAccessReader.java @@ -18,10 +18,14 @@ package org.apache.drill.exec.store.msaccess; +import com.healthmarketscience.jackcess.Database; +import com.healthmarketscience.jackcess.DatabaseBuilder; import org.apache.drill.categories.RowSetTest; +import org.apache.drill.common.exceptions.UserRemoteException; import org.apache.drill.common.types.TypeProtos.MinorType; import org.apache.drill.exec.physical.rowSet.RowSet; import org.apache.drill.exec.physical.rowSet.RowSetBuilder; +import org.apache.drill.exec.proto.UserBitShared.DrillPBError; import org.apache.drill.exec.record.metadata.SchemaBuilder; import org.apache.drill.exec.record.metadata.TupleMetadata; import org.apache.drill.test.ClusterFixture; @@ -33,8 +37,14 @@ import org.junit.experimental.categories.Category; +import java.io.File; + import static org.apache.drill.test.rowSet.RowSetUtilities.strArray; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; @Category(RowSetTest.class) public class TestMSAccessReader extends ClusterTest { @@ -44,6 +54,73 @@ public static void setup() throws Exception { ClusterTest.startCluster(ClusterFixture.builder(dirTestWatcher)); } + /** + * Writes an Access file whose only table is linked to a database that does not exist. The target is + * a path under the test directory so that nothing outside the sandbox (e.g. an SMB share) is touched + * if the link were ever followed. + */ + static File writeLinkedTableFile(String name, String linkedDbPath) throws Exception { + File mdb = new File(dirTestWatcher.getRootDir(), name); + try (Database db = DatabaseBuilder.create(Database.FileFormat.V2003, mdb)) { + db.createLinkedTable("Linked", linkedDbPath, "Table1"); + } + return mdb; + } + + private static File writeLinkedTableFile(String name) throws Exception { + return writeLinkedTableFile(name, new File(dirTestWatcher.getRootDir(), "missing_target.mdb").getAbsolutePath()); + } + + @Test + public void testLinkedTableIsNotResolved() throws Exception { + // A malicious file can point a linked table at any local path or UNC share; reading it must + // not make the Drillbit open that path. + File mdb = writeLinkedTableFile("linked.mdb"); + String sql = "SELECT * FROM table(dfs.`" + mdb.getName() + "` (type=> 'msaccess', tableName => 'Linked'))"; + try { + client.queryBuilder().sql(sql).run(); + fail("Expected the linked database to be refused"); + } catch (UserRemoteException e) { + assertEquals(DrillPBError.ErrorType.PERMISSION, e.getErrorType()); + assertTrue(e.getMessage(), e.getMessage().contains("Table Linked is a linked table")); + assertTrue(e.getMessage(), e.getMessage().contains(MSAccessFormatPlugin.ALLOW_LINKED_DATABASES)); + } + } + + @Test + public void testLinkedTableCannotBeEnabledFromTableFunction() throws Exception { + // Enabling linked tables is a boot option, so a query must not be able to switch it on. + File mdb = writeLinkedTableFile("linked_table_function.mdb"); + String sql = "SELECT * FROM table(dfs.`" + mdb.getName() + + "` (type=> 'msaccess', tableName => 'Linked', allowLinkedDatabases => true))"; + try { + client.queryBuilder().sql(sql).run(); + fail("Expected the query to fail"); + } catch (UserRemoteException e) { + assertNotEquals(e.getMessage(), DrillPBError.ErrorType.DATA_READ, e.getErrorType()); + assertFalse(e.getMessage(), e.getMessage().contains("does not exist")); + } + } + + @Test + public void testMetadataQueryListsLinkedTable() throws Exception { + File mdb = writeLinkedTableFile("linked_metadata.mdb"); + String sql = "SELECT `table`, row_count, col_count FROM dfs.`" + mdb.getName() + "`"; + RowSet results = client.queryBuilder().sql(sql).rowSet(); + + TupleMetadata expectedSchema = new SchemaBuilder() + .add("table", MinorType.VARCHAR) + .addNullable("row_count", MinorType.INT) + .addNullable("col_count", MinorType.INT) + .buildSchema(); + + RowSet expected = new RowSetBuilder(client.allocator(), expectedSchema) + .addRow("Linked", null, null) + .build(); + + new RowSetComparison(expected).verifyAndClearAll(results); + } + @Test public void testStarQuery() throws Exception { String sql = "SELECT * FROM table(cp.`data/V2019/extDateTestV2019.accdb` (type=> 'msaccess', tableName => 'Table1')) LIMIT 5"; @@ -128,16 +205,16 @@ public void testMetadataStarQuery() throws Exception { TupleMetadata expectedSchema = new SchemaBuilder() .add("table", MinorType.VARCHAR) - .add("created_date", MinorType.TIMESTAMP) - .add("updated_date", MinorType.TIMESTAMP) - .add("row_count", MinorType.INT) - .add("col_count", MinorType.INT) + .addNullable("created_date", MinorType.TIMESTAMP) + .addNullable("updated_date", MinorType.TIMESTAMP) + .addNullable("row_count", MinorType.INT) + .addNullable("col_count", MinorType.INT) .addArray("columns", MinorType.VARCHAR) .buildSchema(); RowSet expected = new RowSetBuilder(client.allocator(), expectedSchema) .addRow("Table1", QueryTestUtil.ConvertDateToLong("2021-06-03T20:09:56.993Z"), - QueryTestUtil.ConvertDateToLong("2021-06-03T20:09:56.993Z"), 9, 6, strArray("ID", "Field1", "DateExt", "DateNormal", "DateExtStr", "DateNormalCalc")) + QueryTestUtil.ConvertDateToLong("2021-06-03T20:25:26.483Z"), 9, 6, strArray("ID", "Field1", "DateExt", "DateNormal", "DateExtStr", "DateNormalCalc")) .build(); new RowSetComparison(expected).verifyAndClearAll(results);