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

Application Insights should anonymise SAS tokens in URLs by default #2548

Closed
peter-bertok opened this issue Feb 23, 2022 · 4 comments
Closed
Labels

Comments

@peter-bertok
Copy link

peter-bertok commented Feb 23, 2022

This is related to the issue "Need to anonymise part of information that is logged open-telemetry/opentelemetry-dotnet#1877" that was closed without being resolved.

Many API calls in Azure have query parameters that include secrets such as Storage Account SAS tokens, Azure Function SAS, etc...

The best-practice is to store secrets such as these in Key Vaults for security, which is then undermined by Application Insights because it logs these secrets in plain text and stores them in common log analytics workspaces that are often widely accessible -- e.g.: by developers or consultants. This gives staff with otherwise read-only rights the permission to do "anything" to the target resources, including deleting blobs, altering logs, and triggering functions.

App Insights should anonymize or entirely strip out the "query" part of all logged URL strings by default without requiring any custom code such as "telemetry filters". Provide a configuration option to re-enable the query string logging if needed. Many other web logging systems have this feature.

I need to emphasize that no solution to this problem is viable if it requires an action by customers -- codeless attach is a thing. Even if people are aware of the issue and are using the SDK, the monitored applications can change after Application Insights is deployed. E.g.: developers can add a SAS-based call after the initial review by a security team.

Customers shouldn't have to discover this vulnerability and hand-roll custom fixes as suggested in issue open-telemetry/opentelemetry-dotnet#1877 for basic security measures. For comparison, App Insights already does this kind of filtering by default for SQL query strings, which similarly can contain sensitive information such as PII or financial records.

@peter-bertok
Copy link
Author

Is there any update on this issue?

@cijothomas
Copy link
Contributor

No update.
Stopping to populate query part from url would be a breaking change, and at this stage, the only option is for individual users to write own processing logic.

This is also an issue in OpenTelemetry as seen in this issues:
open-telemetry/opentelemetry-dotnet-contrib#1791
open-telemetry/opentelemetry-dotnet-contrib#1747

@peter-bertok
Copy link
Author

peter-bertok commented May 14, 2022

Stopping to populate query part from url would be a breaking change

Introducing breaking changes for privacy or security is regularly done by the AI team: microsoft/ApplicationInsights-Announcements#28

Similarly, client IP collection used to work by default until it... didn't.

Compared to SQL query collection, query strings in URLs are vastly more dangerous because the former might contain sensitive information, but the latter certainly contains a "full rights" master key in most cases.

This is absurdly dangerous. At the very least the Portal UI should mask/hide the query part automatically, especially for known URLs or known SAS formats!!!

This wouldn't break anyone's code and would go a long way to mitigate issues.

As a real example of how big a security risk this is to Microsoft customers: I noticed this issue while screen sharing to demonstrate App Insights troubleshooting processes with dozens of random developers on the call. Any one of them could have press PrtSc and owned my application. I followed all the best practices of having secrets in KeyVaults, etc... only to have it undermined by a diagnostic tool.

Essentially, I can't hold training sessions any more.

@github-actions
Copy link

This issue is stale because it has been open 300 days with no activity. Remove stale label or this will be closed in 7 days. Commenting will instruct the bot to automatically remove the label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants