Skip to content

Commit

Permalink
Revert "Merged PR 692519: Apply an allowed-list logic to filter out s…
Browse files Browse the repository at this point in the history 
…hared opaque outputs and undeclared source reads"

This reverts commit bf0e495.
  • Loading branch information
@schandr78
schandr78 committed Jan 11, 2023
1 parent 1bac9b1 commit d60758e
Show file tree
Hide file tree
Showing 7 changed files with 131 additions and 427 deletions.
146 changes: 58 additions & 88 deletions Public/Src/Engine/Processes/FileAccessReportingContext.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,32 +80,6 @@ public void ReportFileAccessDeniedByManifest(ReportedFileAccess unexpectedFileAc
MatchAndReportUnexpectedFileAccess(unexpectedFileAccess);
}

/// <summary>
/// Matches an instance of <see cref="ReportedFileAccess"/> with allow list entries.
/// </summary>
public FileAccessAllowlist.MatchType MatchReportedFileAccess(ReportedFileAccess fileAccess) =>
m_fileAccessAllowlist?.HasEntries == true
? m_fileAccessAllowlist.Matches(m_loggingContext, fileAccess, m_pip)
: FileAccessAllowlist.MatchType.NoMatch;

/// <summary>
/// Matches an instance of <see cref="ObservedFileAccess"/> with allow list entries.
/// </summary>
public (FileAccessAllowlist.MatchType aggregateMatchType, (ReportedFileAccess, FileAccessAllowlist.MatchType)[] reportedMatchTypes) MatchObservedFileAccess(ObservedFileAccess observedFileAccess)
{
var aggregateMatch = FileAccessAllowlist.MatchType.MatchesAndCacheable;
var rfas = new (ReportedFileAccess, FileAccessAllowlist.MatchType)[observedFileAccess.Accesses.Count];
int index = 0;
foreach (ReportedFileAccess reportedAccess in observedFileAccess.Accesses)
{
FileAccessAllowlist.MatchType thisMatch = MatchReportedFileAccess(reportedAccess);
rfas[index++] = (reportedAccess, thisMatch);
aggregateMatch = AggregateMatchType(aggregateMatch, thisMatch);
}

return (aggregateMatch, rfas);
}

/// <summary>
/// For an unexpected <see cref="ObservedFileAccess"/> (which is actually an aggregation of <see cref="ReportedFileAccess"/>es to
/// a single path), reports each constituent access and computes an aggregate allowlist match type (the least permissive of any
Expand All @@ -117,83 +91,76 @@ public FileAccessAllowlist.MatchType MatchAndReportUnexpectedObservedFileAccess(
foreach (ReportedFileAccess reportedAccess in unexpectedObservedFileAccess.Accesses)
{
FileAccessAllowlist.MatchType thisMatch = MatchAndReportUnexpectedFileAccess(reportedAccess);
aggregateMatch = AggregateMatchType(aggregateMatch, thisMatch);
}

return aggregateMatch;
}

private static FileAccessAllowlist.MatchType AggregateMatchType(FileAccessAllowlist.MatchType aggregateType, FileAccessAllowlist.MatchType currentType)
{
switch (currentType)
{
case FileAccessAllowlist.MatchType.NoMatch:
aggregateType = FileAccessAllowlist.MatchType.NoMatch;
break;
case FileAccessAllowlist.MatchType.MatchesButNotCacheable:
if (aggregateType == FileAccessAllowlist.MatchType.MatchesAndCacheable)
{
aggregateType = FileAccessAllowlist.MatchType.MatchesButNotCacheable;
}

break;
default:
Contract.Assert(currentType == FileAccessAllowlist.MatchType.MatchesAndCacheable);
break;
switch (thisMatch)
{
case FileAccessAllowlist.MatchType.NoMatch:
aggregateMatch = FileAccessAllowlist.MatchType.NoMatch;
break;
case FileAccessAllowlist.MatchType.MatchesButNotCacheable:
if (aggregateMatch == FileAccessAllowlist.MatchType.MatchesAndCacheable)
{
aggregateMatch = FileAccessAllowlist.MatchType.MatchesButNotCacheable;
}

break;
default:
Contract.Assert(thisMatch == FileAccessAllowlist.MatchType.MatchesAndCacheable);
break;
}
}

return aggregateType;
return aggregateMatch;
}

/// <summary>
/// Reports an access that - ignoring allowlisting - was unexpected. This can be due to a manifest-side or BuildXL-side denial decision.
/// </summary>
private FileAccessAllowlist.MatchType MatchAndReportUnexpectedFileAccess(ReportedFileAccess unexpectedFileAccess)
{
FileAccessAllowlist.MatchType matchType = FileAccessAllowlist.MatchType.NoMatch;

if (m_fileAccessAllowlist != null && m_fileAccessAllowlist.HasEntries)
{
Contract.Assert(
m_config.FailUnexpectedFileAccesses == false,
"Having a file-access allowlist requires that Detours failure injection is off.");

matchType = m_fileAccessAllowlist.Matches(m_loggingContext, unexpectedFileAccess, m_pip);
}

ReportFileAccess(unexpectedFileAccess, matchType);
return matchType;
}
FileAccessAllowlist.MatchType matchType = m_fileAccessAllowlist.Matches(m_loggingContext, unexpectedFileAccess, m_pip);
switch (matchType)
{
case FileAccessAllowlist.MatchType.NoMatch:
AddUnexpectedFileAccessNotAllowlisted(unexpectedFileAccess);
ReportUnexpectedFileAccessNotAllowlisted(unexpectedFileAccess);
break;
case FileAccessAllowlist.MatchType.MatchesButNotCacheable:
AddUnexpectedFileAccessAllowlisted(unexpectedFileAccess);
m_numAllowlistedButNotCacheableFileAccessViolations++;
ReportAllowlistedFileAccessNonCacheable(unexpectedFileAccess);
break;
case FileAccessAllowlist.MatchType.MatchesAndCacheable:
AddUnexpectedFileAccessAllowlisted(unexpectedFileAccess);
m_numAllowlistedAndCacheableFileAccessViolations++;
ReportAllowlistedFileAccessCacheable(unexpectedFileAccess);
break;
default:
throw Contract.AssertFailure("Unknown allowlist-match type.");
}

/// <summary>
/// Reports file access to this reporting context.
/// </summary>
public void ReportFileAccess(ReportedFileAccess fileAccess, FileAccessAllowlist.MatchType matchType)
{
switch (matchType)
return matchType;
}
else
{
case FileAccessAllowlist.MatchType.NoMatch:
AddUnexpectedFileAccessNotAllowlisted(fileAccess);
ReportUnexpectedFileAccessNotAllowlisted(fileAccess);
break;
case FileAccessAllowlist.MatchType.MatchesButNotCacheable:
AddUnexpectedFileAccessAllowlisted(fileAccess);
m_numAllowlistedButNotCacheableFileAccessViolations++;
ReportAllowlistedFileAccessNonCacheable(fileAccess);
break;
case FileAccessAllowlist.MatchType.MatchesAndCacheable:
AddUnexpectedFileAccessAllowlisted(fileAccess);
m_numAllowlistedAndCacheableFileAccessViolations++;
ReportAllowlistedFileAccessCacheable(fileAccess);
break;
default:
throw Contract.AssertFailure("Unknown allowlist-match type.");
AddUnexpectedFileAccessNotAllowlisted(unexpectedFileAccess);
ReportUnexpectedFileAccessNotAllowlisted(unexpectedFileAccess);
return FileAccessAllowlist.MatchType.NoMatch;
}
}

private void AddUnexpectedFileAccessNotAllowlisted(ReportedFileAccess reportedFileAccess)
{
m_violations ??= new List<ReportedFileAccess>();
if (m_violations == null)
{
m_violations = new List<ReportedFileAccess>();
}

if (reportedFileAccess.Operation != ReportedFileOperation.NtCreateFile || m_config.UnsafeSandboxConfiguration.MonitorNtCreateFile)
{
Expand All @@ -208,7 +175,10 @@ private void AddUnexpectedFileAccessNotAllowlisted(ReportedFileAccess reportedFi

private void AddUnexpectedFileAccessAllowlisted(ReportedFileAccess reportedFileAccess)
{
m_allowlistedAccesses ??= new List<ReportedFileAccess>();
if (m_allowlistedAccesses == null)
{
m_allowlistedAccesses = new List<ReportedFileAccess>();
}

if (reportedFileAccess.Operation != ReportedFileOperation.NtCreateFile || m_config.UnsafeSandboxConfiguration.MonitorNtCreateFile)
{
Expand All @@ -223,7 +193,7 @@ private void ReportUnexpectedFileAccessNotAllowlisted(ReportedFileAccess reporte

if (path.StartsWith(PipEnvironment.RestrictedTemp, OperatingSystemHelper.PathComparison))
{
Tracing.Logger.Log.PipProcessDisallowedTempFileAccess(
BuildXL.Processes.Tracing.Logger.Log.PipProcessDisallowedTempFileAccess(
m_loggingContext,
m_pip.SemiStableHash,
m_pip.GetDescription(m_context),
Expand All @@ -232,7 +202,7 @@ private void ReportUnexpectedFileAccessNotAllowlisted(ReportedFileAccess reporte
}
else
{
Tracing.Logger.Log.PipProcessDisallowedFileAccess(
BuildXL.Processes.Tracing.Logger.Log.PipProcessDisallowedFileAccess(
m_loggingContext,
m_pip.SemiStableHash,
m_pip.GetDescription(m_context),
Expand All @@ -242,13 +212,13 @@ private void ReportUnexpectedFileAccessNotAllowlisted(ReportedFileAccess reporte
path);

if (reportedFileAccess.Operation == ReportedFileOperation.NtCreateFile &&
!m_config.UnsafeSandboxConfiguration.MonitorNtCreateFile)
!m_config.UnsafeSandboxConfiguration.MonitorNtCreateFile)
{
// If the unsafe_IgnoreNtCreate is set, disallowed ntCreateFile accesses are not marked as violations.
// Since there will be no error or warning for the ignored NtCreateFile violations in the FileMonitoringViolationAnalyzer,
// this is the only place for us to log a warning for those.
// We also need to emit a dx09 verbose above for those violations due to WrapItUp.
Tracing.Logger.Log.PipProcessDisallowedNtCreateFileAccessWarning(
BuildXL.Processes.Tracing.Logger.Log.PipProcessDisallowedNtCreateFileAccessWarning(
m_loggingContext,
m_pip.SemiStableHash,
m_pip.GetDescription(m_context),
Expand All @@ -267,7 +237,7 @@ private void ReportAllowlistedFileAccessNonCacheable(ReportedFileAccess reported

if (m_reportAllowlistedAccesses)
{
Tracing.Logger.Log.PipProcessUncacheableAllowlistNotAllowedInDistributedBuilds(
BuildXL.Processes.Tracing.Logger.Log.PipProcessUncacheableAllowlistNotAllowedInDistributedBuilds(
m_loggingContext,
m_pip.SemiStableHash,
m_pip.GetDescription(m_context),
Expand All @@ -278,7 +248,7 @@ private void ReportAllowlistedFileAccessNonCacheable(ReportedFileAccess reported
}
else
{
Tracing.Logger.Log.PipProcessDisallowedFileAccessAllowlistedNonCacheable(
BuildXL.Processes.Tracing.Logger.Log.PipProcessDisallowedFileAccessAllowlistedNonCacheable(
m_loggingContext,
m_pip.SemiStableHash,
m_pip.GetDescription(m_context),
Expand All @@ -292,7 +262,7 @@ private void ReportAllowlistedFileAccessCacheable(ReportedFileAccess reportedFil
string path = reportedFileAccess.GetPath(m_context.PathTable);
string description = reportedFileAccess.Describe();

Tracing.Logger.Log.PipProcessDisallowedFileAccessAllowlistedCacheable(
BuildXL.Processes.Tracing.Logger.Log.PipProcessDisallowedFileAccessAllowlistedCacheable(
m_loggingContext,
m_pip.SemiStableHash,
m_pip.GetDescription(m_context),
Expand Down
Loading

0 comments on commit d60758e

Please sign in to comment.