Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IAST: support Spring Webflux #4942

Merged
merged 1 commit into from
Apr 12, 2023
Merged

IAST: support Spring Webflux #4942

merged 1 commit into from
Apr 12, 2023

Conversation

cataphract
Copy link
Contributor

No description provided.

@cataphract cataphract requested a review from a team March 22, 2023 10:08
@cataphract cataphract requested a review from a team as a code owner March 22, 2023 10:08
@cataphract cataphract added the comp: asm iast Application Security Management (IAST) label Mar 22, 2023
@cataphract cataphract force-pushed the glopes/spring-boot-path-parameter-taint branch from 61ecb0f to 65d914f Compare March 22, 2023 10:15
@cataphract cataphract force-pushed the glopes/spring-boot-path-parameter-taint branch from 65d914f to 9ab5fde Compare March 22, 2023 10:24
@cataphract cataphract force-pushed the glopes/spring-boot-path-parameter-taint branch from 9ab5fde to 9412cef Compare March 22, 2023 15:00
@cataphract cataphract force-pushed the glopes/spring-boot-path-parameter-taint branch from 9412cef to 9adcd5d Compare March 22, 2023 15:18
Base automatically changed from glopes/spring-boot-path-parameter-taint to master March 22, 2023 18:27
@cataphract cataphract force-pushed the glopes/iast-spring-webflux branch 2 times, most recently from fce6045 to 4c2e62a Compare March 23, 2023 09:47
for (Map.Entry<String, List<String>> e :
((MultiValueMap<String, String>) values).entrySet()) {
String lc = e.getKey().toLowerCase(Locale.ENGLISH);
module.onHeaderName(lc);

Choose a reason for hiding this comment

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

We taint by reference so the local variable lc might defeat the purpose of the tainted map, we should only taint the original string instances.

In any case, I think in here it's more suitable a wrapped Map/MultiValueMap instance that taints on demand (for example, the keys from the dictionary are rarely used by the app as they are often hard coded constants). Maybe we can come back to it for the next iteration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, the tainting of the transformed key makes no sense, I've fixed it.

re: the second point, this advice is precisely for when the user injects a full map (or multimap) of headers, so I think it makes sense to taint it all. If he was interested in only one or two headers, he'd likely inject only those specific ones into string variables.

@@ -96,6 +96,19 @@ public void taint(final byte origin, @Nullable final Object... toTaintArray) {
}
}

@Override
public boolean isTainted(@Nullable Object obj) {
final TaintedObjects taintedObjects = getTaintedObjects();

Choose a reason for hiding this comment

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

The interface Taintable can be used for non bootclasspath classes (where we can add fields) and it also marks an object as tainted (maybe we should deal with this abstraction inside the TaintedObjects?)

Copy link
Contributor Author

@cataphract cataphract Mar 31, 2023

Choose a reason for hiding this comment

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

The current version now checks first if the object is taintable:

  @Override
  public boolean isTainted(@Nullable Object obj) {
    if (obj instanceof Taintable) {
      return ((Taintable) obj).$DD$isTainted();
    }

    // ...
  }

I think doing it in TaintedObjects is too late; by then we've already fetched the context and traversed a few pointers, which is not needed.

This is done in https://github.com/DataDog/dd-trace-java/pull/4996/files

@cataphract cataphract force-pushed the glopes/iast-spring-webflux branch 2 times, most recently from e33eaa0 to 4e9c442 Compare March 31, 2023 10:09
@cataphract cataphract requested a review from smola March 31, 2023 10:18
@cataphract cataphract changed the base branch from master to glopes/common-iast March 31, 2023 10:18
Base automatically changed from glopes/common-iast to master April 3, 2023 09:28
@smola smola changed the title IAST: support webflux IAST: support Spring Webflux Apr 3, 2023
@SuppressWarnings("Duplicates")
@Advice.OnMethodExit(suppress = Throwable.class)
public static void after(
@Advice.Argument(2) ServerWebExchange xchg, @ActiveRequestContext RequestContext reqCtx) {
Copy link
Member

Choose a reason for hiding this comment

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

Can xchg possibly be null? If there's the slightest chance of it, I will check it first and do early return. But I guess it would not be the case for onExit...

@cataphract cataphract force-pushed the glopes/iast-spring-webflux branch 2 times, most recently from ddb1169 to 2bb1cce Compare April 10, 2023 09:31
@cataphract cataphract merged commit c47397d into master Apr 12, 2023
@cataphract cataphract deleted the glopes/iast-spring-webflux branch April 12, 2023 18:26
@github-actions github-actions bot added this to the 1.12.0 milestone Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp: asm iast Application Security Management (IAST)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants