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

Make it possible to disallow slow queries (or warn user about the problem) #315

Merged
merged 8 commits into from
Jan 10, 2023
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
22 changes: 22 additions & 0 deletions docs/yara.md
Original file line number Diff line number Diff line change
Expand Up @@ -215,3 +215,25 @@ the rule could cripple mquery performance.
Remember that parser is your friend. If your query runs too slow, click "parse" instead
of "query" and investigate if the query looks reasonable.

## Slow Yara rules.

Some yara rules cannot be optimised by mquery and will end up scanning the whole
malware collection. One example of such rule is:

```
rule UnluckyExample
{
strings:
$code = {48 8b 0? 0f b6 c? 48 8b 4? 34 50}

condition:
all of them and pe.imphash() == "b8bb385806b89680e13fc0cf24f4431e"
}
```

This is not necessarily a bad rule, but there's not a single full 3gram that can
be used to narrow the set of suspected files. Due to how mquery works, this will
yara scan every malware file in the dataset, and will be very slow. Becaue of this,
such queries are by defauly disasllowed. They can be enabled by setting
`query_allow_slow` config key to true. In this case mquery will allow such
queries, but it'll ask for confirmation first.
25 changes: 24 additions & 1 deletion src/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ def query(
)

if not rules:
raise HTTPException(status_code=400, detail=f"No rule was specified.")
raise HTTPException(status_code=400, detail="No rule was specified.")

if data.method == RequestQueryMethod.parse:
return [
Expand All @@ -335,11 +335,34 @@ def query(
rule_author=rule.author,
is_global=rule.is_global,
is_private=rule.is_private,
is_degenerate=rule.parse().is_degenerate,
Copy link
Member

Choose a reason for hiding this comment

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

We could probably call rule.parse() just once, somewhere above? In case it gets expensive in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Performance hit is negliglible, and even if it wasn't, it's already cached:

    def parse(self) -> UrsaExpression:
        if self.__parsed is None:
            self.__parsed = self.__parse_internal()
        return self.__parsed

I worry more about readability. There's no way to assign it to a temporary variable in a list comprehension, but I can rewrite this all to an explicit for loop if you prefer 🤔 It would look like this:

response_rules = []
for rule in rules:
    parsed_rule = rule.parse()
    response_rules.append(
        ParseResponseSchema(
            rule_name=rule.name,
            rule_author=rule.author,
            is_global=rule.is_global,
            is_private=rule.is_private,
            is_degenerate=parsed_rule.is_degenerate,
            parsed=parsed_rule.query,
        )
    )
return response_rules

I prefer the current version, but I'm not overly attached to it if you prefer this one.

parsed=rule.parse().query,
)
for rule in rules
]

degenerate_rules = [r.name for r in rules if r.parse().is_degenerate]
allow_slow = db.get_mquery_config_key("query_allow_slow") == "true"
msm-code marked this conversation as resolved.
Show resolved Hide resolved
if degenerate_rules and not (allow_slow and data.force_slow_queries):
if allow_slow:
# Warning: "You can force a slow query" literal is used to
# pattern match on the error message in the frontend.
help_message = "You can force a slow query if you want."
else:
help_message = "This is not allowed by this server."
degenerate_rule_names = ", ".join(degenerate_rules)
doc_url = "https://cert-polska.github.io/mquery/docs/yara.html"
raise HTTPException(
status_code=400,
detail=(
"Invalid query. Some of the rules require a full Yara scan of"
"every indexed file. "
f"{help_message} "
f"Slow rules: {degenerate_rule_names}. "
f"Read {doc_url} for more details."
Copy link
Collaborator

Choose a reason for hiding this comment

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

link is not clickable, I guess it should :)

Copy link
Contributor Author

@msm-code msm-code Dec 29, 2022

Choose a reason for hiding this comment

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

Not possible with an error message (without severe hacks, like putting HTML code into a Python exception, or - even worse - parsing the error message on the frontend).

I guess I'll do it the harder way, and put the related alert logic into the frontend. This will require some changes, so I'll address this a bit later (in this PR).

),
)

active_agents = db.get_active_agents()

for agent, agent_spec in active_agents.items():
Expand Down
2 changes: 2 additions & 0 deletions src/db.py
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,8 @@ def get_core_config(self) -> Dict[str, str]:
"openid_url": "OpenID Connect base url",
"openid_client_id": "OpenID client ID",
"openid_secret": "Secret used for JWT token verification",
# Query and performance config
"query_allow_slow": "Allow users to run queries that will end up scanning the whole malware collection",
}

def get_config(self) -> List[ConfigSchema]:
Expand Down
1 change: 1 addition & 0 deletions src/mqueryfront/src/config/ConfigEntries.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const KNOWN_RULES = {
openid_url: R_URL,
auth_enabled: R_BOOL,
auth_default_roles: R_ROLES,
query_allow_slow: R_BOOL,
};

class ConfigRow extends Component {
Expand Down
1 change: 1 addition & 0 deletions src/mqueryfront/src/query/QueryField.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const QueryField = (props) => (
onTaintSelect={props.onTaintSelect}
availableTaints={props.availableTaints}
selectedTaints={props.selectedTaints}
forceSlowQueries={props.forceSlowQueries}
/>
<div className="mt-2 monaco-container">
<QueryMonaco
Expand Down
20 changes: 11 additions & 9 deletions src/mqueryfront/src/query/QueryLayoutManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,9 @@ const QueryLayoutManager = (props) => {
onYaraUpdate,
parsedError,
selectedTaints,
forceSlowQueries,
} = props;

const queryParse = queryError ? (
<ErrorPage error={queryError} />
) : queryPlan ? (
<QueryParseStatus queryPlan={queryPlan} />
) : null;

const queryResults = job ? (
<div>
<button
Expand All @@ -57,10 +52,16 @@ const QueryLayoutManager = (props) => {
<LoadingPage />
);

const queryResultOrParse = qhash ? queryResults : queryParse;
const resultsTab = queryError ? (
<ErrorPage error={queryError} />
) : queryPlan ? (
<QueryParseStatus queryPlan={queryPlan} />
) : qhash ? (
queryResults
) : null;

const queryFieldPane = isCollapsed ? null : (
<div className={queryResultOrParse ? "col-md-6" : "col-md-12"}>
<div className={resultsTab ? "col-md-6" : "col-md-12"}>
<QueryField
readOnly={!!qhash}
onSubmitQuery={onSubmitQuery}
Expand All @@ -72,6 +73,7 @@ const QueryLayoutManager = (props) => {
onYaraUpdate={onYaraUpdate}
parsedError={parsedError}
selectedTaints={selectedTaints}
forceSlowQueries={forceSlowQueries}
/>
</div>
);
Expand All @@ -87,7 +89,7 @@ const QueryLayoutManager = (props) => {
: "col-md-6 order-first order-md-last"
}
>
{queryResultOrParse}
{resultsTab}
</div>
</div>
</div>
Expand Down
6 changes: 5 additions & 1 deletion src/mqueryfront/src/query/QueryNavigation.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,15 @@ const QueryNavigation = (props) => {
isEditActive,
availableTaints,
selectedTaints,
forceSlowQueries,
} = props;

return (
<div className="btn-group" role="group">
<QuerySubmitNav onClick={onSubmitQuery} />
<QuerySubmitNav
onClick={onSubmitQuery}
forceMode={forceSlowQueries}
/>
<QueryEditParseNav
isEditActive={isEditActive}
onEditQuery={onEditQuery}
Expand Down
11 changes: 11 additions & 0 deletions src/mqueryfront/src/query/QueryPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ const INITIAL_STATE = {
selectedTaints: [],
job: null,
activePage: 1,
forceSlowQueries: false,
};

const PAGE_SIZE = 20;
Expand Down Expand Up @@ -94,6 +95,9 @@ class QueryPageInner extends Component {
};

handleYaraUpdate(value) {
this.setState({
forceSlowQueries: false,
});
this.setState({ rawYara: value });
}

Expand Down Expand Up @@ -167,6 +171,7 @@ class QueryPageInner extends Component {
method: method,
priority: priority,
taints: taints,
force_slow_queries: this.state.forceSlowQueries,
});
if (method === "query") {
this.props.navigate(`/query/${response.data.query_hash}`);
Expand All @@ -183,6 +188,11 @@ class QueryPageInner extends Component {
: error.toString(),
queryPlan: null,
});
if (this.state.queryError.match(/You can force a slow query/)) {
this.setState({
forceSlowQueries: true,
});
}
}
}

Expand Down Expand Up @@ -249,6 +259,7 @@ class QueryPageInner extends Component {
onYaraUpdate={this.handleYaraUpdate}
parsedError={this.parsedError}
selectedTaints={this.state.selectedTaints}
forceSlowQueries={this.state.forceSlowQueries}
/>
</ErrorBoundary>
);
Expand Down
30 changes: 22 additions & 8 deletions src/mqueryfront/src/query/QueryParseStatus.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,28 @@ const QueryParseStatus = (props) => {
if (!queryPlan) return null;

const parseResult = queryPlan.map((rule) => {
const { is_private, is_global, rule_name, parsed } = rule;
const {
is_private,
is_global,
is_degenerate,
rule_name,
parsed,
} = rule;

const badge =
is_private || is_global ? (
<span className="badge badge-info">
{is_private ? "private" : "global"}
</span>
) : null;
const private_badge = is_private ? (
<span className="badge bg-info">private</span>
) : null;
const global_badge = is_global ? (
<span className="badge bg-info">private</span>
) : null;
const degenerate_badge = is_degenerate ? (
<span className="badge bg-danger">degenerate</span>
) : null;
const badges = (
<>
{private_badge} {global_badge} {degenerate_badge}
</>
);

return (
<div key={rule_name} className="mt-3">
Expand All @@ -22,7 +36,7 @@ const QueryParseStatus = (props) => {
<span className="me-2 font-weight-bold">
{rule_name}
</span>
{badge}
{badges}
</label>
<div className="jumbotron text-monospace text-break p-2">
{parsed}
Expand Down
16 changes: 9 additions & 7 deletions src/mqueryfront/src/query/QuerySubmitNav.js
Original file line number Diff line number Diff line change
@@ -1,23 +1,25 @@
import React from "react";

const QuerySubmitNav = (props) => {
const { onClick } = props;
const { onClick, forceMode } = props;

const label = forceMode ? "Force query (may be very slow!)" : "Query";
const style = forceMode ? "btn-danger" : "btn-success";

return (
<React.Fragment>
<button
type="button"
className="btn btn-success btn-sm"
className={"btn btn-sm " + style}
onClick={() => onClick("medium")}
>
Query
{label}
</button>
<div className="btn-group" role="group">
<div className="btn-group">
<button
type="button"
className="btn btn-success dropdown-toggle"
data-toggle="dropdown"
aria-haspopup="true"
className={"btn dropdown-toggle " + style}
data-bs-toggle="dropdown"
aria-expanded="false"
/>
<div className="dropdown-menu">
Expand Down
2 changes: 1 addition & 1 deletion src/mqueryfront/src/status/BackendStatus.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class AgentStatus extends Component {
let badge = null;
if (!this.props.alive) {
badge = (
<span className="badge badge-secondary badge-sm">offline</span>
<span className="badge bg-secondary badge-sm">offline</span>
);
}

Expand Down
2 changes: 2 additions & 0 deletions src/schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ class QueryRequestSchema(BaseModel):
files_limit: Optional[int]
reference: Optional[str] # arbitrary data specified by the user
required_plugins: List[str] = Field([])
force_slow_queries: bool = False


class QueryResponseSchema(BaseModel):
Expand All @@ -76,6 +77,7 @@ class ParseResponseSchema(BaseModel):
rule_author: str
is_global: bool
is_private: bool
is_degenerate: bool
parsed: str


Expand Down