security: escape Graphviz exec args & validate DBSearch::unserialize …

[nikunj-kohli]

Base information

Question	Answer
Related to a SourceForge thead / Another PR / Combodo ticket?
Type of change?	Bug fix / Enhancement

Symptom (bug) / Objective (enhancement)

• Symptom:
  • The Graphviz lifecycle endpoint (graphviz.php) built a shell command using input redirection and unescaped path components. In some misconfigured environments or with tampered configuration values, this increased the risk of shell injection when rendering lifecycle graphs.
  • DBSearch::unserialize() accepted decoded payloads (urlencoded JSON containing OQL and params) without structural validation or size limits. Since the decoded data flows through multiple request code paths, malformed or excessively large payloads could be processed unexpectedly, with unclear failure modes.
• Objective:
  • Reduce attack surface by (1) building the Graphviz (dot) invocation using safely quoted arguments and removing unsafe shell input redirection, and (2) validating the structure and size of payloads passed to DBSearch::unserialize() and rejecting invalid or oversized inputs early and cleanly.

Reproduction procedure (bug)

(If you need me to provide exact curl commands or a small test script for CI, I can add them — I kept examples high-level here so they are safe for public PR discussion.)

Environment assumptions (please replace with the exact values you want to put in the form if different):

• iTop: develop branch (current PR target)
• PHP: typical supported versions (e.g., 7.4, 8.0, 8.1, 8.2). Replace with your exact tested PHP version in the table below.

Graphviz unsafe-path scenario:

1. On a standard iTop instance (develop branch).
2. Set graphviz_path in your configuration to an unexpected or malicious value that includes shell metacharacters (for instance: a path containing spaces or characters that the shell would interpret).
   • Example misconfiguration (do not run as-is on a production host): set graphviz_path to a value containing shell metacharacters.
3. Request lifecycle image generation:
   • Open: pages/graphviz.php?class=SomeExistingClass
4. Before this patch:
   • The server built a command line that used input redirection and unescaped args; a malicious graphviz_path could cause the shell to execute unintended tokens.
5. With this patch:
   • The dot command is built with escaped arguments and input is passed safely; injection via graphviz_path is prevented.

DBSearch unserialize malformed/oversized payload:

1. Identify an endpoint that accepts a filter parameter encoded as urlencoded JSON and passed to DBSearch::unserialize() (for example, certain ajax operations in ajax.render.php).
2. Normal case:
   • Send a valid filter parameter: e.g., filter=%7B%22oql%22%3A%22SELECT%20Person%20WHERE%20name%20LIKE%20%27%25A%25%27%22%2C%22params%22%3A%5B%5D%7D
   • Expected: behavior unchanged — the search runs and returns results.
3. Invalid case:
   • Send filter that is not urlencoded JSON (e.g., filter=notjson) or a very large payload (e.g., an encoded OQL where the oql field contains > 20,000 characters).
   • Before this patch: the code attempted to decode and use the payload which could lead to unexpected behaviors or heavy processing.
   • After this patch: request fails fast with a CoreException and an "Invalid filter parameter" style error, avoiding further processing of malformed/oversized payloads.

Cause (bug)

• Graphviz:
  • The lifecycle endpoint constructed a shell command using input redirection (< "$file") and concatenated path components without proper quoting. This allows shell interpretation of certain injected characters in a configuration or input value.
• DBSearch:
  • DBSearch::unserialize() parsed a urlencoded JSON payload (expected to contain oql and params) but did not validate the decoded structure or impose limits on the size of the contained OQL string. Because this codepath is used in multiple places (including request-driven ajax endpoints), malformed or extremely large payloads could be passed into DBSearch without clear validation.

Proposed solution (bug and enhancement)

What I implemented in this PR:

• graphviz.php

  • Build the dot execution command using safely quoted arguments (PHP-side escaping) rather than relying on shell input redirection.
  • Remove the < "$file" style redirection that previously fed the dot input via the shell.
  • Redirect stderr to stdout safely while keeping the whole command argument-quoted.
  • Rationale: even if graphviz_path (the configured path to dot) or other path components were modified or malformed, properly quoting the arguments prevents the shell from interpreting injected tokens.

• dbsearch.class.php

  • Add strict validation in DBSearch::unserialize():
    • Ensure the decoded payload is an array with an oql key (string) and a params key (array).
    • Impose a maximum length on the oql string (e.g., 20_000 characters — reasonable defensive limit; can be tuned).
    • Throw a CoreException with a clear message (e.g., "Invalid filter parameter") when the payload is malformed or exceeds limits.
  • Rationale: rejects malformed or oversized serialized filters early and consistently, avoiding unexpected behavior downstream.

Behavioral notes:

• Functionality preserved for legitimate requests:
  • Graph rendering remains unchanged when graphviz_path and inputs are valid.
  • DBSearch still accepts the same encoded OQL format (urlencoded JSON) for valid payloads; invalid payloads now cause a clear immediate exception.
• Defensive-by-default:
  • These are low-risk, localized changes intended to improve security and fail early with clear errors.

Files changed

• graphviz.php — build dot command with escaped arguments; remove shell < file redirection. (See patch)
• dbsearch.class.php — add structure/type checks and length limit on decoded oql; ensure params is an array; throw CoreException on invalid payloads. (See patch)

Checklist before requesting a review

• [x] I have performed a self-review of my code
• [ ] I have tested all changes I made on an iTop instance
  • Notes: manual smoke checks are suggested (see "How to test locally" below). If you want, I can add an automated scenario or a small integration test demonstrating the error behavior.
• [ ] I have added a unit test, otherwise I have explained why I couldn't
  • Notes: This change is defensive input validation and a command invocation change; unit tests can be added around DBSearch::unserialize() easily (happy to add a small PHPUnit test), and an integration smoke test can exercise graphviz.php if a dot binary is available in CI.
• [x] Is the PR clear and detailed enough so anyone can understand digging in the code?

Checklist of things to do before PR is ready to merge

• [ ] Add a small unit test for DBSearch::unserialize() for:
  • valid payload,
  • malformed JSON / wrong structure,
  • oversized oql hitting the limit.
• [ ] Add a short integration/manual check note in the PR (how to validate graphviz command invocation in CI or locally).
• [ ] Address any review comments about the exact oql length limit (current limit is a conservative default and can be tuned).
• [ ] If maintainers prefer a different error type or handling (log + fallback), adapt behavior accordingly.

[jf-cbd]

Hello, I have a few questions about your PR : Could you please use the default template ? Could you also give me a context, in iTop, where this could happen ? Look like this PR is global, not for iTop (where the context can maybe make it useless), so I'd appreciate that you can give us more details, thank you in advance For example, $escapedDotOutput = escapeshellarg($sImageFilePath); seems useless since it's built with methods like utils::GetDataPath() that doesn't come from the user. Or maybe I missed something, like a different way to bypass it ?

[nikunj-kohli]

Hello, I have a few questions about your PR : Could you please use the default template ? Could you also give me a context, in iTop, where this could happen ? Look like this PR is global, not for iTop (where the context can maybe make it useless), so I'd appreciate that you can give us more details, thank you in advance For example, $escapedDotOutput = escapeshellarg($sImageFilePath); seems useless since it's built with methods like utils::GetDataPath() that doesn't come from the user. Or maybe I missed something, like a different way to bypass it ?

Hi @jf-cbd, thank you for the review! Let me address your questions:

Context where these issues could occur in iTop:

1. Graphviz command injection (graphviz.php)

You're correct that $sImageFilePath is built internally via utils::GetDataPath(). However, $sDotExecutable comes from:

$sDotExecutable = MetaModel::GetConfig()->Get('graphviz_path');

This config value could potentially contain unexpected characters if:

• An administrator with config access (but not necessarily full server access) sets a malicious or improperly formatted path
• The config file is modified by another vulnerability or misconfiguration
• A compromised extension or module modifies the config

While the risk is lower since it requires config-level access, using escapeshellarg() provides defense-in-depth and follows secure coding best practices for any external command execution.

If you prefer, I can:

• Remove the escaping for $sImageFilePath (since it's internally generated)
• Keep only $sDotExecutable and $sDotFilePath escaped
• Update the PR description to clarify the actual attack surface

2. DBSearch::unserialize validation (dbsearch.class.php)

This is called from multiple user-facing endpoints that accept serialized filter parameters:

• ajax.render.php (operations: 'ajax', 'pie_chart', 'chart') - lines 617, 656, 712, 343
• UI.php (multiple filter operations) - lines 530, 732, 782, 833, 860, 1054, 1573, 1604, 1630
• run_query.php - line 127
• tagadmin.php - line 87

Example attack scenario:

1. User sends a crafted request to pages/ajax.render.php?operation=ajax&filter=<malformed_payload>
2. Without validation, malformed JSON structures or extremely large payloads could cause unexpected behavior
3. The added checks reject invalid structures early with a clear error

Current code path:

$sFilter = utils::ReadParam('filter', '', false, 'raw_data');
$oFilter = DBSearch::unserialize($sFilter); // User-controlled input

The validation ensures the decoded payload has the expected structure before processing.

Re: Default PR template

Could you point me to the template location? I'll update the PR description to follow it.

Proposed action:

Would you prefer I:

1. Simplify the graphviz fix - only escape $sDotExecutable (the config value) and $sDotFilePath, remove $sImageFilePath escaping?
2. Keep the DBSearch validation as-is (addresses real user input)?
3. Update PR description with the template and more specific context?

Let me know and I'll update accordingly. Thanks for the careful review!

[Hipska]

@nikunj-kohli The template was already linked to by @jf-cbd: https://github.com/Combodo/iTop/blob/develop/.github/pull_request_template.md

Please don't use AI to generate PRs, replies and code changes, it puts more burden on maintainers and doesn't help. It is the opposite of helping out and against rules of #Hacktoberfest. Especially these last lines give it away, since that was what the AI agent told You to do, it was not ment to paste this in the PR description:

Rollback / mitigation

• Reverting the commit(s) restores previous behavior. Both changes are isolated and low-risk; if maintainers prefer a different validation policy for DBSearch::unserialize() we can tune the checks or extract validation into a helper for easier adjustment.

Suggested commit message

• security: escape Graphviz exec args and validate DBSearch::unserialize input

  • Escape shell args when running dot to avoid injection; add structure/type/length checks in DBSearch::unserialize() to reject malformed or oversized filter payloads.

Suggested reviewers / labels

• reviewers: core-maintainers, security-team
• labels: security, bug, quickfix

Optional git commands (PowerShell) to create branch, commit and push (copy/paste locally)

git checkout -b fix/graphviz-escape-dbsearch-unserialize-validation
git add pages/graphviz.php core/dbsearch.class.php
git commit -m "security: escape Graphviz exec args and validate DBSearch::unserialize input"
git push --set-upstream origin fix/graphviz-escape-dbsearch-unserialize-validation

Notes / next steps I can take after you open the PR

• Prepare a concise PR checklist (CI steps, manual checks) and a short unit/regression test or example request for maintainers.
• If maintainers accept, proceed to the next item in the queue: mitigate or replace eval() usage in renderer JS (high priority), or progressively harden unserialize call sites (wrap with try/catch and reject request filters explicitly).

[nikunj-kohli]

@nikunj-kohli The template was already linked to by @jf-cbd: https://github.com/Combodo/iTop/blob/develop/.github/pull_request_template.md

Please don't use AI to generate PRs, replies and cod, it puts more burden on maintainers and doesn't help. It is the opposite of helping out and against rules of #Hacktoberfest. Especially these last lines give it away, since that was what the AI agent told You to do, it was not ment to paste this in the PR description:

Rollback / mitigation

• Reverting the commit(s) restores previous behavior. Both changes are isolated and low-risk; if maintainers prefer a different validation policy for DBSearch::unserialize() we can tune the checks or extract validation into a helper for easier adjustment.

Suggested commit message

• security: escape Graphviz exec args and validate DBSearch::unserialize input

  • Escape shell args when running dot to avoid injection; add structure/type/length checks in DBSearch::unserialize() to reject malformed or oversized filter payloads.

Suggested reviewers / labels

• reviewers: core-maintainers, security-team
• labels: security, bug, quickfix

Optional git commands (PowerShell) to create branch, commit and push (copy/paste locally)

git checkout -b fix/graphviz-escape-dbsearch-unserialize-validation
git add pages/graphviz.php core/dbsearch.class.php
git commit -m "security: escape Graphviz exec args and validate DBSearch::unserialize input"
git push --set-upstream origin fix/graphviz-escape-dbsearch-unserialize-validation

Notes / next steps I can take after you open the PR

• Prepare a concise PR checklist (CI steps, manual checks) and a short unit/regression test or example request for maintainers.
• If maintainers accept, proceed to the next item in the queue: mitigate or replace eval() usage in renderer JS (high priority), or progressively harden unserialize call sites (wrap with try/catch and reject request filters explicitly).

Okay, I'll keep that in my mind. Do you want me to change the format right now or the current one is bearable?

[Hipska]

Since this part is the first section of the template, I would say "no, not bearable".

<!--

IMPORTANT: Please follow the guidelines within this PR template before submitting it, it will greatly help us process your PR. 🙏

Any PRs not following the guidelines or with missing information will not be considered.

-->

Unless you want your PR not to be considered..

[nikunj-kohli]

Since this part is the first section of the template, I would say "no, not bearable".

<!--

IMPORTANT: Please follow the guidelines within this PR template before submitting it, it will greatly help us process your PR. 🙏

Any PRs not following the guidelines or with missing information will not be considered.

-->

Unless you want your PR not to be considered..

Alright, I've made the change in PR Description. You can check and let me know if I'm still making a mistake, thankyou for your review.

[jf-cbd]

Thank you for the changes. Could you please explain me, with your words, the 3 different "protections" you implemented, and give an example of how to exploit the non-corrected code in iTop ? For example, how to perform the "Example attack scenario" you described ? It seems it's already protected, no ?