fix(pii): Do not panic on large regex [INC-202] #1474

jjbayer · 2022-09-15T07:12:47Z

In INC-202 we saw that project configs with large entries for sensitiveFields are able to crash worker threads, because we compile the list into a string with | and did not handle the CompiledTooBig error returned by the regex crate.

This PR prevents the panic by making to_pii_config fallible. The resulting failure mode is that calling scrub_data on events will fail, and the event is dropped with outcome "internal".

See also getsentry/sentry#38803.

jjbayer · 2022-09-15T07:13:27Z

relay-cabi/src/processing.rs

+        Ok(Some(config)) => RelayStr::from_string(config.to_json()?),
+        Ok(None) => RelayStr::new("{}"),
+        // NOTE: Callers of this function must be able to handle this error.
+        Err(e) => RelayStr::from_string(e.to_string()),


The sentry side does not handle this case yet.

jjbayer · 2022-09-15T07:13:52Z

relay-general/benches/benchmarks.rs

@@ -140,7 +140,7 @@ fn bench_pii_stripping(c: &mut Criterion) {
        |b, datascrubbing_config| b.iter(|| datascrubbing_config.pii_config_uncached()),
    );

-    let pii_config = datascrubbing_config.pii_config_uncached().unwrap();
+    let pii_config = datascrubbing_config.pii_config_uncached().unwrap().unwrap();


This is just a benchmark, so unwrap should be fine.

jjbayer · 2022-09-15T07:14:26Z

relay-general/src/pii/config.rs

+impl Pattern {
+    pub fn new(s: &str, case_insensitive: bool) -> Result<Self, PiiConfigError> {
+        let regex = RegexBuilder::new(s)
+            .size_limit(262_144)


Apply the same size limit to every Pattern that we create.

Why that size limit specifically?

Also: should we make the number a constant?

I moved it from [here, but it's a valid question. It's 2^18, and it seems we already copied it from somewhere else once: https://github.com//pull/505/files#r388342341

@untitaker any recollection where this value comes from? My guess is it was arbitrary from the start. We can definitely make it a constant though.

jjbayer · 2022-09-15T07:15:06Z

relay-general/src/pii/convert.rs

-                            RegexBuilder::new(&key_pattern)
-                                .case_insensitive(true)
-                                .build()
-                                .unwrap(),


This was the line that panicked in production.

iker-barriocanal · 2022-09-15T07:49:34Z

relay-general/src/pii/config.rs

+impl Pattern {
+    pub fn new(s: &str, case_insensitive: bool) -> Result<Self, PiiConfigError> {
+        let regex = RegexBuilder::new(s)
+            .size_limit(262_144)


Why that size limit specifically?

iker-barriocanal · 2022-09-15T07:52:26Z

relay-general/src/pii/config.rs

+impl Pattern {
+    pub fn new(s: &str, case_insensitive: bool) -> Result<Self, PiiConfigError> {
+        let regex = RegexBuilder::new(s)
+            .size_limit(262_144)


Also: should we make the number a constant?

py/CHANGELOG.md

CHANGELOG.md

relay-general/src/pii/config.rs

relay-general/src/pii/convert.rs

relay-general/src/pii/config.rs

jan-auer · 2022-09-15T09:13:24Z

relay-server/src/actors/processor.rs

+            let pii_config = config
+                .datascrubbing_settings
+                .pii_config()
+                .map_err(|e| ProcessingError::PiiConfigError(e.clone()))?;


This will drop the event, which we should assign a dedicated discard reason to (or use the one we also use for broken project configs). Can we additionally mark the project config as invalid somehow so we reject subsequent events earlier?

This got me thinking, the entire concept of holding a cached Result inside the config, and then failing every time an event needs it, seems wrong.

relay/relay-general/src/pii/legacy.rs

Line 41 in 8dbbc85

pub(super) pii_config: OnceCell<Result<Option<PiiConfig>, PiiConfigError>>,

I simplified the code to convert the config on serialization, thereby making this particular error a deserialization error. Because it's a bigger refactor, I put that change into its own sub-PR: #1478

Closed the linked PR (see discussion there) and added a separate discard reason for pii conversion failure.

I would not put extra effort into invalidating the project config when this conversion fails, since having invalid data scrubbing settings is a clear bug that should be fixed on the sentry side, rather than an acceptable day-to-day case.

Co-authored-by: Jan Michael Auer <[email protected]> Co-authored-by: Iker Barriocanal <[email protected]>

INC-202 showed that allowing the user to configure arbitrary sensitive fields will exceed the maximum limit on the compiled regex in Relay. This PR limits sensitiveFields to 4000 characters. See also getsentry/relay#1474.

untitaker · 2022-09-16T08:29:00Z

relay-general/src/pii/legacy.rs

@@ -37,6 +37,10 @@ pub struct DataScrubbingConfig {
    /// PII config derived from datascrubbing settings.
    ///
    /// Cached because the conversion process is expensive.
+    ///
+    /// NOTE: We discarded the idea of making the conversion to PiiConfig part of deserialization,


untitaker · 2022-09-16T08:30:05Z

relay-general/src/pii/config.rs

 use once_cell::sync::OnceCell;
 use regex::{Regex, RegexBuilder};
 use serde::{de::Error, Deserialize, Deserializer, Serialize, Serializer};

 use crate::pii::{CompiledPiiConfig, Redaction};
 use crate::processor::SelectorSpec;

+const COMPILED_PATTERN_MAX_SIZE: usize = 262_144;


this max size was determined by trial and error so that we are able to parse builtin rules at all. I think it was either the creditcard or ip addr regex that required increasing the limit

#1474 introduced an explicit size limit on the regex used to convert legacy datascrubbing settings to PII config. We are seeing new outcomes with reason project_state_pii in our statsd metrics now, so we should log an error to Sentry (tagged by project_id) to see which projects are affected.

This is a partial revert of #1474. Instead of applying a stricter size limit to regexes built from datascrubbing settings, which led to data loss for at least one project, only keep the part that prevents a panic on CompiledSizeTooBig.

jjbayer added 6 commits September 13, 2022 17:14

ref: Apply size limit in Pattern::new

68b0f21

test: Write a test, see it fail

df2a9f2

fix: Make to_pii_config fallible

ed619bc

fix: Remaining function calls

ab04eaa

changelog and update test

64b0abd

Merge remote-tracking branch 'origin/master' into fix/pii-regex-unwrap

3105d85

jjbayer commented Sep 15, 2022

View reviewed changes

jjbayer marked this pull request as ready for review September 15, 2022 07:43

jjbayer requested a review from a team September 15, 2022 07:43

iker-barriocanal approved these changes Sep 15, 2022

View reviewed changes

jjbayer mentioned this pull request Sep 15, 2022

fix(datascrubbing): Limit sensitiveFields [INC-202] getsentry/sentry#38803

Merged

jan-auer reviewed Sep 15, 2022

View reviewed changes

jjbayer and others added 2 commits September 15, 2022 11:35

Apply suggestions from code review

434c99c

Co-authored-by: Jan Michael Auer <[email protected]> Co-authored-by: Iker Barriocanal <[email protected]>

PR feedback

8dbbc85

jjbayer mentioned this pull request Sep 15, 2022

ref(pii): Convert datascrubbing settings on deserialization [INC-202] #1478

Closed

jjbayer added 2 commits September 16, 2022 09:49

ref: Separate discard reason for pii conversion failure, doc comment

69a7dbe

Merge remote-tracking branch 'origin/master' into fix/pii-regex-unwrap

a7623fc

jjbayer requested a review from jan-auer September 16, 2022 08:02

untitaker approved these changes Sep 16, 2022

View reviewed changes

jjbayer merged commit 4cd8084 into master Sep 16, 2022

jjbayer deleted the fix/pii-regex-unwrap branch September 16, 2022 11:14

jjbayer mentioned this pull request Sep 16, 2022

instr(pii): Log error on failed config conversion [INC-202] #1480

Merged

jjbayer mentioned this pull request Sep 16, 2022

fix(pii): Revert regex limit for datascrubbing conversion [INGEST-1616] #1481

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(pii): Do not panic on large regex [INC-202] #1474

fix(pii): Do not panic on large regex [INC-202] #1474

jjbayer commented Sep 15, 2022 •

edited

Loading

jjbayer Sep 15, 2022

jjbayer Sep 15, 2022

jjbayer Sep 15, 2022

iker-barriocanal Sep 15, 2022

iker-barriocanal Sep 15, 2022

jjbayer Sep 15, 2022

jjbayer Sep 15, 2022

iker-barriocanal Sep 15, 2022

iker-barriocanal Sep 15, 2022

jan-auer Sep 15, 2022

jjbayer Sep 15, 2022

jjbayer Sep 16, 2022

untitaker Sep 16, 2022

untitaker Sep 16, 2022

fix(pii): Do not panic on large regex [INC-202] #1474

fix(pii): Do not panic on large regex [INC-202] #1474

Conversation

jjbayer commented Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjbayer commented Sep 15, 2022 •

edited

Loading