Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"database/sql"
"fmt"
"log/slog"
"strconv"

"github.com/prometheus/client_golang/prometheus"
"go.uber.org/atomic"
Expand All @@ -21,13 +22,26 @@ const selectIndexIOWaits = `
WHERE INDEX_NAME IS NOT NULL AND OBJECT_SCHEMA NOT IN %s`

// mysql.innodb_index_stats holds InnoDB's persistent optimizer statistics,
// refreshed by MySQL itself.
// refreshed by MySQL itself. NON_UNIQUE comes from information_schema.statistics,
// joined on seq_in_index = 1 since uniqueness is a property of the index, not of
// any one column within it, and would otherwise repeat once per indexed column.
const selectIndexSizeBytes = `
SELECT database_name, table_name, index_name, stat_value * @@innodb_page_size
FROM mysql.innodb_index_stats
WHERE stat_name = 'size' AND database_name NOT IN %s`
SELECT
s.database_name,
s.table_name,
s.index_name,
s.stat_value * @@innodb_page_size,
stats.NON_UNIQUE
FROM mysql.innodb_index_stats s
LEFT JOIN information_schema.statistics stats
ON stats.TABLE_SCHEMA = s.database_name
AND stats.TABLE_NAME = s.table_name
AND stats.INDEX_NAME = s.index_name
AND stats.SEQ_IN_INDEX = 1
WHERE s.stat_name = 'size' AND s.database_name NOT IN %s`

var indexLabels = []string{labelSchema, labelTable, "index"}
var indexSizeLabels = append(append([]string{}, indexLabels...), "is_primary", "is_unique")

var (
indexStatsIdxFetchDesc = prometheus.NewDesc(
Expand All @@ -37,8 +51,8 @@ var (
)
indexStatsSizeBytesDesc = prometheus.NewDesc(
prometheus.BuildFQName("database_observability", "mysql_index_stats", "size_bytes"),
"Total disk space used by this index, in bytes",
indexLabels, nil,
"Total disk space used by this index, in bytes, labeled with whether it backs the primary key or a unique constraint",
indexSizeLabels, nil,
)
)

Expand Down Expand Up @@ -142,13 +156,19 @@ func (c *IndexStats) collectIndexSize(ctx context.Context, ch chan<- prometheus.
for rows.Next() {
var databaseName, tableName, indexName string
var sizeBytes float64
var nonUnique sql.NullInt64

if err := rows.Scan(&databaseName, &tableName, &indexName, &sizeBytes); err != nil {
if err := rows.Scan(&databaseName, &tableName, &indexName, &sizeBytes, &nonUnique); err != nil {
c.logger.Error("failed to scan mysql.innodb_index_stats row", "err", err)
return
}

ch <- prometheus.MustNewConstMetric(indexStatsSizeBytesDesc, prometheus.GaugeValue, sizeBytes, databaseName, tableName, indexName)
isPrimary := indexName == "PRIMARY"
isUnique := nonUnique.Valid && nonUnique.Int64 == 0

ch <- prometheus.MustNewConstMetric(indexStatsSizeBytesDesc, prometheus.GaugeValue, sizeBytes,
databaseName, tableName, indexName,
strconv.FormatBool(isPrimary), strconv.FormatBool(isUnique))
}

if err := rows.Err(); err != nil {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,17 +37,19 @@ func TestIndexStats(t *testing.T) {
)
mock.ExpectQuery(fmt.Sprintf(selectIndexSizeBytes, exclusionClause)).WithoutArgs().RowsWillBeClosed().
WillReturnRows(
sqlmock.NewRows([]string{"database_name", "table_name", "index_name", "size_bytes"}).
AddRow("books_store", "books", "idx_books_title", 14196736),
sqlmock.NewRows([]string{"database_name", "table_name", "index_name", "size_bytes", "non_unique"}).
AddRow("books_store", "books", "PRIMARY", 65536, 0).
AddRow("books_store", "books", "idx_books_title", 14196736, 1),
)

expected := `
# HELP database_observability_mysql_index_stats_idx_fetch_total Count of index I/O wait events for fetch operations
# TYPE database_observability_mysql_index_stats_idx_fetch_total counter
database_observability_mysql_index_stats_idx_fetch_total{index="idx_books_title",schema="books_store",table="books"} 0
# HELP database_observability_mysql_index_stats_size_bytes Total disk space used by this index, in bytes
# HELP database_observability_mysql_index_stats_size_bytes Total disk space used by this index, in bytes, labeled with whether it backs the primary key or a unique constraint
# TYPE database_observability_mysql_index_stats_size_bytes gauge
database_observability_mysql_index_stats_size_bytes{index="idx_books_title",schema="books_store",table="books"} 1.4196736e+07
database_observability_mysql_index_stats_size_bytes{index="PRIMARY",is_primary="true",is_unique="true",schema="books_store",table="books"} 65536
database_observability_mysql_index_stats_size_bytes{index="idx_books_title",is_primary="false",is_unique="false",schema="books_store",table="books"} 1.4196736e+07
`

require.NoError(t, testutil.CollectAndCompare(registry, strings.NewReader(expected)))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,32 @@ const selectTableIOWaitsNoIndex = `
FROM performance_schema.table_io_waits_summary_by_index_usage
WHERE INDEX_NAME IS NULL AND OBJECT_SCHEMA NOT IN %s`

// mysql.innodb_table_stats holds InnoDB's persistent optimizer statistics,
// refreshed by MySQL itself -- the sibling table to mysql.innodb_index_stats,
// already used by index_stats.go for index size.
const selectTableRowCount = `
SELECT database_name, table_name, n_rows
FROM mysql.innodb_table_stats
WHERE database_name NOT IN %s`

const (
labelSchema = "schema"
labelTable = "table"
)

var tableStatsNoIdxFetchDesc = prometheus.NewDesc(
prometheus.BuildFQName("database_observability", "mysql_table_stats", "no_idx_fetch_total"),
"Count of index I/O wait events for fetch operations that did not use an index",
[]string{labelSchema, labelTable}, nil,
var tableLabels = []string{labelSchema, labelTable}

var (
tableStatsNoIdxFetchDesc = prometheus.NewDesc(
prometheus.BuildFQName("database_observability", "mysql_table_stats", "no_idx_fetch_total"),
"Count of index I/O wait events for fetch operations that did not use an index",
tableLabels, nil,
)
tableStatsRowCountDesc = prometheus.NewDesc(
prometheus.BuildFQName("database_observability", "mysql_table_stats", "row_count"),
"Estimated number of rows in this table",
tableLabels, nil,
)
)

type TableStatsArguments struct {
Expand Down Expand Up @@ -82,12 +99,18 @@ func (c *TableStats) Stop() {
// Describe implements prometheus.Collector.
func (c *TableStats) Describe(ch chan<- *prometheus.Desc) {
ch <- tableStatsNoIdxFetchDesc
ch <- tableStatsRowCountDesc
}

// Collect implements prometheus.Collector. It runs synchronously at scrape time.
func (c *TableStats) Collect(ch chan<- prometheus.Metric) {
ctx := context.Background()

c.collectNoIdxFetch(ctx, ch)
c.collectRowCount(ctx, ch)
}

func (c *TableStats) collectNoIdxFetch(ctx context.Context, ch chan<- prometheus.Metric) {
query := fmt.Sprintf(selectTableIOWaitsNoIndex, buildExcludedSchemasClause(c.excludeSchemas))
rows, err := c.dbConnection.QueryContext(ctx, query)
if err != nil {
Expand All @@ -112,3 +135,29 @@ func (c *TableStats) Collect(ch chan<- prometheus.Metric) {
c.logger.Error("error iterating table_io_waits_summary_by_index_usage rows", "err", err)
}
}

func (c *TableStats) collectRowCount(ctx context.Context, ch chan<- prometheus.Metric) {
query := fmt.Sprintf(selectTableRowCount, buildExcludedSchemasClause(c.excludeSchemas))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semgrep identified a blocking 🔴 issue in your code:

collectRowCount interpolates c.excludeSchemas into SQL before QueryContext executes it. Malicious schema configuration could alter the filter and expose data through the metrics queries.

More details about this

collectRowCount builds the SQL text with fmt.Sprintf(selectTableRowCount, buildExcludedSchemasClause(c.excludeSchemas)) and then executes that text through c.dbConnection.QueryContext. The value returned by buildExcludedSchemasClause is inserted into the SQL before the database receives it, rather than being passed as a query parameter.

If c.excludeSchemas can be influenced by deployment configuration or another attacker-controlled input, an attacker could supply a schema value such as tenant_a') OR 1=1 -- . If buildExcludedSchemasClause places that value inside a quoted NOT IN clause, the generated query could change from an exclusion filter into a condition that matches every row. When the metrics collection path calls collectRowCount, QueryContext would then return row counts for schemas that were intended to be excluded; a payload crafted to add a UNION or additional statement could expose other database data if the MySQL driver and account permit it. The same construction pattern is also present in collectNoIdxFetch with selectTableIOWaitsNoIndex and c.excludeSchemas, so the impact may include both table row-count and index-usage metrics.

To resolve this comment:

🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.

💬 Ignore this finding

Reply with Semgrep commands to ignore this finding.

  • /fp <comment> for false positive
  • /ar <comment> for acceptable risk
  • /other <comment> for all other reasons

Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by string-formatted-query.

We're currently testing semgrep's diff-aware PR comment feature on a subset of our repos-- if you run into issues or find this spammy, please reach out to @danny.cooper in slack and give feedback.

For backwards compatability with gosec, its best to use polyglot suppression comments of the following format for false positives:
// #nosec <gosec rule ID> nosemgrep: <semgrep rule ID>

You can view more details about this finding in the Semgrep AppSec Platform.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/fp this is running inside an alloy instance and the param is fully under the user control via config file, so the risk is rather low.

rows, err := c.dbConnection.QueryContext(ctx, query)
if err != nil {
c.logger.Error("failed to query mysql.innodb_table_stats", "err", err)
return
}
defer rows.Close()

for rows.Next() {
var databaseName, tableName string
var rowCount int64

if err := rows.Scan(&databaseName, &tableName, &rowCount); err != nil {
c.logger.Error("failed to scan mysql.innodb_table_stats row", "err", err)
return
}

ch <- prometheus.MustNewConstMetric(tableStatsRowCountDesc, prometheus.GaugeValue, float64(rowCount), databaseName, tableName)
}

if err := rows.Err(); err != nil {
c.logger.Error("error iterating mysql.innodb_table_stats rows", "err", err)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,19 @@ func TestTableStats(t *testing.T) {
sqlmock.NewRows([]string{"OBJECT_SCHEMA", "OBJECT_NAME", "COUNT_FETCH"}).
AddRow("books_store", "books", 39),
)
mock.ExpectQuery(fmt.Sprintf(selectTableRowCount, exclusionClause)).WithoutArgs().RowsWillBeClosed().
WillReturnRows(
sqlmock.NewRows([]string{"database_name", "table_name", "n_rows"}).
AddRow("books_store", "books", 500),
)

expected := `
# HELP database_observability_mysql_table_stats_no_idx_fetch_total Count of index I/O wait events for fetch operations that did not use an index
# TYPE database_observability_mysql_table_stats_no_idx_fetch_total counter
database_observability_mysql_table_stats_no_idx_fetch_total{schema="books_store",table="books"} 39
# HELP database_observability_mysql_table_stats_row_count Estimated number of rows in this table
# TYPE database_observability_mysql_table_stats_row_count gauge
database_observability_mysql_table_stats_row_count{schema="books_store",table="books"} 500
`

require.NoError(t, testutil.CollectAndCompare(registry, strings.NewReader(expected)))
Expand Down
Loading