Skip to content

fix: use bounded formatting for Vorbis metadata names#1250

Merged
ddennedy merged 3 commits into
mltframework:masterfrom
orbisai0security:fix-vorbis-sprintf-buffer-overflow
May 27, 2026
Merged

fix: use bounded formatting for Vorbis metadata names#1250
ddennedy merged 3 commits into
mltframework:masterfrom
orbisai0security:fix-vorbis-sprintf-buffer-overflow

Conversation

@orbisai0security
Copy link
Copy Markdown
Contributor

@orbisai0security orbisai0security commented May 26, 2026

Summary

Replace sprintf() with snprintf() when constructing Vorbis metadata attribute names.

The destination buffer meta->name is dynamically allocated based on the input key length plus the fixed format overhead (strlen(str) + 18), so the current code is already sized correctly. However, using snprintf() is a defensive hardening improvement that:

  • Makes the formatting operation explicitly bounded
  • Protects against potential future changes to allocation logic
  • Follows secure coding best practices

Changes

  • src/modules/vorbis/producer_vorbis.c - Replace sprintf with snprintf
  • src/tests/test_vorbis/test_vorbis.cpp - Add QTest-based test following MLT conventions

Verification

  • Build passes
  • Test follows QTest framework (per maintainer feedback)
  • Test registered in CMakeLists.txt

Technical Details

Before:

meta->name = malloc(strlen(str) + 18);
sprintf(meta->name, "meta.attr.%s.markup", str);

After:

meta->name = malloc(strlen(str) + 18);
snprintf(meta->name, strlen(str) + 18, "meta.attr.%s.markup", str);

The format string overhead is 18 bytes:

  • "meta.attr." = 10 bytes
  • ".markup" = 7 bytes
  • null terminator = 1 byte

This change makes the code more robust and follows defense-in-depth principles, even though the allocation is already appropriately sized.


Automated security fix by OrbisAI Security

Automated security fix generated by OrbisAI Security
The Vorbis producer module uses sprintf() to format a metadata attribute name from a Vorbis file's comment tags into a fixed-size buffer (meta->name) without any bounds checking
Copy link
Copy Markdown
Member

@ddennedy ddennedy left a comment

Choose a reason for hiding this comment

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

Thank you for the report and contribution. The included test does not conform to this repository's choice of QTest for unit tests. See src/tests/test_mod_avformat/ for an example. The project also uses CMake; so a new test directory should be added to src/tests/CMakeLists.txt.

@orbisai0security orbisai0security changed the title fix: use snprintf in producer_vorbis.c fix: use bounded formatting for Vorbis metadata names May 27, 2026
Replace Check-based test with QTest following MLT conventions:
- Remove tests/test_invariant_producer_vorbis.c (Check framework)
- Add src/tests/test_vorbis/test_vorbis.cpp (QTest framework)
- Register test in src/tests/CMakeLists.txt under MOD_VORBIS

The new test follows the pattern established in test_mod_avformat and
integrates with MLT's existing QTest/CMake infrastructure.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor Author

@orbisai0security orbisai0security left a comment

Choose a reason for hiding this comment

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

Thanks for the review and for pointing me to the project's test conventions. I agree the current test should not introduce a different unit-test framework.

I've revised this PR to:

  1. Migrate the test to QTest framework, following the pattern in src/tests/test_mod_avformat/
  2. Register the test in src/tests/CMakeLists.txt under the MOD_VORBIS conditional
  3. Update the PR description to accurately reflect that meta->name is dynamically allocated with malloc(strlen(str) + 18)
  4. Reframe this as defensive hardening rather than fixing a critical overflow

You're absolutely right that the original wording about "fixed-size buffer" was incorrect — the allocation is sized appropriately based on the input string length. The snprintf change is primarily about making this code path more robust against future changes and following secure coding best practices.

The test is now much simpler and follows MLT's conventions, focusing on integration testing through the public API rather than trying to simulate the vulnerability with synthetic buffers.

@ddennedy ddennedy merged commit 1595ef0 into mltframework:master May 27, 2026
15 of 17 checks passed
@ddennedy ddennedy added this to the v7.40.0 milestone May 27, 2026
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.

2 participants