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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 8 commits into from
Jan 10, 2023
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
175417f
Disallow degenerate queries
msm-code Dec 27, 2022
527aacd
Disallow degenerate queries
msm-code Dec 29, 2022
fcb9088
Update src/app.py
msm-code Dec 29, 2022
b23a95d
WIP allow force-running a slow query (and merge)
msm-code Jan 4, 2023
8a9f337
Calm down flake8
msm-code Jan 4, 2023
8318280
Some name changes
msm-code Jan 5, 2023
0dc4f1b
Check slow config on the backend too
msm-code Jan 5, 2023
581a043
remove unnecessary comment
msm-code Jan 5, 2023
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
19 changes: 18 additions & 1 deletion src/app.py
View file
Open in desktop
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,28 @@ 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]
disallow_degenerate = db.get_mquery_config_key("query_disallow_degenerate")
if degenerate_rules and (disallow_degenerate == "true"):
msm-code marked this conversation as resolved.
Show resolved Hide resolved
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 would require a Yara scan of every indexed "
"file, and this is not allowed by this instance. "
f"Problematic 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 •
edited
Loading

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
View file
Open in desktop
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_disallow_degenerate": "Reject any 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
View file
Open in desktop
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_disallow_degenerate: R_BOOL,
};

class ConfigRow extends Component {
Expand Down
30 changes: 22 additions & 8 deletions src/mqueryfront/src/query/QueryParseStatus.js
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion src/mqueryfront/src/status/BackendStatus.js
View file
Open in desktop
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
1 change: 1 addition & 0 deletions src/schema.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ class ParseResponseSchema(BaseModel):
rule_author: str
is_global: bool
is_private: bool
is_degenerate: bool
parsed: str


Expand Down