How buffer overflow via unchecked memcpy offset happens in C++ PCL point cloud parsing and how to fix it

Introduction

In the PostgreSQL Pointcloud extension's PCL integration module, we discovered a critical out-of-bounds read vulnerability in contrib/pcl/src/pcpatch_pcl.cpp at line 400. The readFloat lambda function—used to extract floating-point values from point cloud row data—performed a memcpy operation using an offset value directly from PCLPointField structures without any validation that the read would stay within buffer boundaries.

This vulnerability is particularly dangerous because point cloud data files (PCD format) are often processed from external sources. An attacker who can supply a crafted PCD file with malicious PCLPointField structures containing oversized offset values could trigger reads beyond allocated memory, potentially leaking sensitive information or crashing the application.

The Vulnerability Explained

The vulnerable code resided in the pcpatch_from_pcd function, which converts PCL point cloud data into PostgreSQL's internal format. Here's the problematic lambda:

auto readFloat = [](const pcl::PCLPointField* f, const uint8_t* row) -> float {
    float v;
    std::memcpy(&v, row + f->offset, sizeof(float));
    return v;
};

The issue is straightforward but critical: f->offset comes from the parsed PCLPointField structure, which is populated from the input PCD file. The code blindly trusts this offset value and uses it to calculate the source address for memcpy.

How the Attack Works

Consider this attack scenario:

1. An attacker creates a malicious PCD file where the PCLPointField structure for the "x" coordinate has an offset value of 0xFFFFFF00 (a very large number)
2. When pcpatch_from_pcd processes this file, it calls findField("x") which returns the malicious field structure
3. The readFloat lambda is invoked with this field and a legitimate row buffer (say, 32 bytes)
4. The calculation row + f->offset produces a pointer far beyond the allocated buffer
5. memcpy reads 4 bytes from this invalid memory location

The consequences depend on what memory lies at that address:
- Information disclosure: Reading adjacent heap data could leak sensitive information like encryption keys, authentication tokens, or other users' data
- Denial of service: Reading unmapped memory triggers a segmentation fault, crashing the PostgreSQL backend
- Heap metadata corruption: In some scenarios, the read could interact with heap management structures

Why This Pattern is Dangerous

The PR notes that similar patterns exist at lines 405 and 432, suggesting this was a systemic issue in how the codebase handled field offsets. Any time you're computing memory addresses using values from external input, you must validate those values first.

The Fix

The fix adds a crucial bounds check to the readFloat lambda. Here's the before and after comparison:

Before (vulnerable):

auto readFloat = [](const pcl::PCLPointField* f, const uint8_t* row) -> float {
    float v;
    std::memcpy(&v, row + f->offset, sizeof(float));
    return v;
};

After (secure):

auto readFloat = [&cloud2](const pcl::PCLPointField* f, const uint8_t* row) -> float {
    if (f->offset + sizeof(float) > cloud2.point_step)
        throw std::runtime_error("PCLPointField offset out of row buffer bounds");
    float v;
    std::memcpy(&v, row + f->offset, sizeof(float));
    return v;
};

Key Changes Explained

1. Lambda capture modification: The lambda now captures cloud2 by reference ([&cloud2]) to access the authoritative buffer size information

2. Bounds validation: Before any memory access, the code checks that f->offset + sizeof(float) doesn't exceed cloud2.point_step. The point_step field represents the actual size of each point's data row in bytes

3. Fail-safe behavior: If the bounds check fails, the code throws a std::runtime_error with a descriptive message, preventing the dangerous memory access entirely

This three-line change transforms a critical memory safety vulnerability into a handled error condition. The fix is minimal, targeted, and doesn't impact performance for legitimate inputs since the check is a simple integer comparison.

Prevention & Best Practices

Defensive Coding for Memory Operations

1. Always validate offsets before use: Any offset value from external input must be validated against known buffer sizes before being used in pointer arithmetic

2. Use the pattern: offset + size <= buffer_length: This is the canonical bounds check. Note the use of <= for the comparison—if offset + size equals buffer_length exactly, you're reading the last valid bytes

3. Watch for integer overflow: When adding offset and size, ensure the addition itself doesn't overflow. For very large offset values, offset + sizeof(float) could wrap around to a small number, bypassing the check. Consider using safe integer arithmetic libraries

Code Review Checklist

When reviewing code that handles external data formats:

• [ ] Are all offset/index values validated before use?
• [ ] Is the buffer size known and accessible at the point of validation?
• [ ] Could integer overflow bypass the bounds check?
• [ ] Are similar patterns used elsewhere in the codebase that need the same fix?

Static Analysis Integration

Tools that can detect this vulnerability pattern:
- Clang Static Analyzer: Can identify some unchecked buffer access patterns
- Coverity: Excellent at finding buffer overflows in C/C++ code
- AddressSanitizer (ASan): Runtime detection during testing
- Semgrep: Custom rules can match the memcpy(dest, src + untrusted_offset, ...) pattern

Key Takeaways

• Never trust offset values from parsed file formats—the PCLPointField.offset came directly from user-supplied PCD files and should have been validated immediately
• The readFloat lambda at line 400 was a ticking time bomb—any PCD file with a crafted offset could trigger memory corruption
• Similar vulnerable patterns at lines 405 and 432 indicate systemic issues—when you find one instance of a bug pattern, search for others
• The fix leverages existing metadata (cloud2.point_step) rather than introducing new tracking—always look for authoritative size information already available in your data structures
• Throwing an exception on invalid input is the right choice—it's better to fail loudly than to silently corrupt memory

How Orbis AppSec Detected This

• Source: Untrusted PCLPointField.offset value parsed from external PCD point cloud files
• Sink: std::memcpy(&v, row + f->offset, sizeof(float)) in contrib/pcl/src/pcpatch_pcl.cpp:400
• Missing control: No validation that offset + sizeof(float) falls within the row buffer bounds defined by cloud2.point_step
• CWE: CWE-125 (Out-of-bounds Read)
• Fix: Added bounds check comparing f->offset + sizeof(float) against cloud2.point_step before the memcpy operation, throwing std::runtime_error on violation

Orbis AppSec automatically detected this vulnerability and opened a pull request with the fix. Try Orbis AppSec on your repositories to find and fix issues like this automatically.

Conclusion

This vulnerability demonstrates a fundamental principle of secure C++ programming: never use values from external input to calculate memory addresses without validation. The readFloat lambda's direct use of f->offset in pointer arithmetic created a critical security flaw that could be exploited through crafted PCD files.

The fix is elegant in its simplicity—a three-line change that validates the offset against the known row size before any memory access occurs. This pattern of "validate before use" should be applied consistently whenever handling structured data from untrusted sources.

For developers working with binary file formats, point cloud data, or any code that parses structured external input: audit your offset and size handling carefully. The memory safety guarantees of modern C++ don't protect you when you're doing raw pointer arithmetic with untrusted values.

References

• CWE-125: Out-of-bounds Read
• OWASP Buffer Overflow Prevention Cheat Sheet
• C++ Core Guidelines: Bounds Safety
• Semgrep C/C++ Buffer Overflow Rules
• fix: add bounds check before memcpy in pcpatch_pcl.cpp