Skip to content

Comments

Fix vulnerabilities#226

Open
k4amos wants to merge 2 commits intoAD-Security:mainfrom
k4amos:fix_vulnerabilities
Open

Fix vulnerabilities#226
k4amos wants to merge 2 commits intoAD-Security:mainfrom
k4amos:fix_vulnerabilities

Conversation

@k4amos
Copy link
Collaborator

@k4amos k4amos commented Dec 3, 2025

This PR addresses several minor security vulnerabilities in AD_Miner (cf. #225)

  • Unsafe pickle deserialization: Replaced pickle.load() with a restricted unpickler that only allows whitelisted
    classes, preventing arbitrary code execution from malicious cache files
  • Cross-Site Scripting (XSS): Added HTML escaping for all Neo4j data rendered in generated HTML reports, preventing
    script injection through malicious AD object names

Changes

Pickle deserialization hardening:

  • Added safe_pickle.py with RestrictedUnpickler implementing a strict class whitelist
  • Updated cache_class.py and analyse_cache.py to use safe_load() instead of pickle.load()
  • Added path traversal validation in analyse_cache.py

XSS prevention:

  • Added escape_html() utility function in utils.py
  • Updated grid_data_stringify() to escape dynamic values
  • Fixed ~34 control files to escape Neo4j data before HTML concatenation
  • Added JavaScript escaping functions in search_bar.js and graph.js
  • Secured common_analysis.py, main_page.py, and table_class.py

Risk

Without this fix:

  • An attacker with write access to the Neo4j database could inject malicious JavaScript through AD object names
    (users, computers, groups, domains)
  • Malicious cache files could execute arbitrary Python code when loaded

@LeBaronDeCharlus
Copy link

👍

@snowpeacock
Copy link
Collaborator

Hello, thanks for the contribution.

The pickle deserialization hardening seems ok.
Regarding the XSS we will try to find a global solution, as manually fixing each control may leave gaps.

Copy link
Collaborator

@snowpeacock snowpeacock left a comment

Choose a reason for hiding this comment

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

XSS

@k4amos
Copy link
Collaborator Author

k4amos commented Jan 15, 2026

Hi @snowpeacock,

Thank you for the review. As you know, HTML is generated through multiple patterns:

  • Template substitution with %s (table_class.py, card_class.py, grid_class.py)
  • String concatenation in many control files
  • F-strings in main_page.py, line_class.py

The current fix manually applies escape_html() across all these locations but I agree that a unified solution could be preferable, for example using a new template that would cover all these different cases and would automatically escape the variables.

Here's what this approach could look like for each pattern :

case 1 (e.g, table_class.py)

# Before
page_f.write(header_f.read() % (self.id, self.id, self.title, self.id))

# After  
template = SafeTemplate.from_file(header_path)
page_f.write(template.render(
    id=self.id,
    title=self.title,
    icon=raw('<i class="bi bi-shield"></i>')
))

case 2 (e.g, card_class.py)

# Before
page_f.write(html_header % (self.color, self.title, self.icon))

# After
template = SafeTemplate(html_header)
page_f.write(template.render(
    raw(self.color),
    self.title,
    raw(self.icon)
))

case 3 (e.g, smolcard_class.py)

# Before - custom fillTemplate() method
html_line = self.fillTemplate(html_raw, template_data)

# After 
template = SafeTemplate(html_raw)
html_line = template.render(**template_data)

case 4 (e.g, grid_class.py)

# Before
new_contents = template_contents.replace("// DATA PLACEHOLDER", textToInsert)

# After
template = SafeTemplate(template_contents)
result = template.replace("// DATA PLACEHOLDER", textToInsert)
page_f.write(str(result))

case 5 (e.g, controls/*.py)

# Before
temp_data["name"] = '<i class="bi bi-server"></i> ' + d["name"]

# After : helper function 
temp_data["name"] = icon_label("bi bi-server", d["name"])

# Helper in safe_template.py
def icon_label(icon_class, label):
    """Shortcut for the common icon + label pattern."""
    return f'<i class="{icon_class}"></i> {SafeTemplate._escape(label)}'

What do you think? Is this the kind of approach you had in mind?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants