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.

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

CAN: fix length calculation (fixes #12311) #12441

Merged
merged 1 commit into from
Feb 19, 2020

Conversation

daniel-starke
Copy link
Contributor

Fix for issue #12311: Potential Buffer Overrun in CANMessage Constructor

The two types of the CANMessage constructor accepting a data buffer have two issues. First, they limit the input buffer size to the 4 least significant bits of the passed length even though a CAN message cannot have more than 8 bytes of payload. Second, the used data length in the following memcpy() uses the initially passed data length which may exceed the internal data buffer size. Both will lead into hard to find bugs if the passed data buffer size is outside the limits according to the CAN standard. This fix intends to solve this by limiting the input data size to 8 bytes.

Impact of changes

There is no known impact.

Migration actions required

None.

Documentation

None. The API did not change from the user's point of view.


Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[ ] Feature update (New feature / Functionality change / New API)
[ ] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[x] No Tests required for this change (E.g docs only update)
[ ] Covered by existing mbed-os tests (Greentea or Unittest)
[ ] Tests / results supplied as part of this PR

No test is required but advised to avoid future regressions.


Reviewers

@0xc0170 This is my first contribution to this project. Please kindly review this change.


@ciarmcom ciarmcom requested review from a team February 14, 2020 22:00
@ciarmcom
Copy link
Member

@daniel-starke, thank you for your changes.
@ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

@mergify mergify bot added needs: CI and removed needs: review labels Feb 17, 2020
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

Can you amend this to the commit message:

The two types of the CANMessage constructor accepting a data buffer have two issues. First, they limit the input buffer size to the 4 least significant bits of the passed length even though a CAN message cannot have more than 8 bytes of payload. Second, the used data length in the following memcpy() uses the initially passed data length which may exceed the internal data buffer size. Both will lead into hard to find bugs if the passed data buffer size is outside the limits according to the CAN standard. This fix intends to solve this by limiting the input data size to 8 bytes.

just add another section there. Plus also add module name if you can to the first line. The proposal:

CAN: fix length calculation in constructionr

The two types of the CANMessage constructor accepting a data buffer have two issues. First, they limit the input buffer size to the 4 least significant bits of the passed length even though a CAN message cannot have more than 8 bytes of payload. Second, the used data length in the following memcpy() uses the initially passed data length which may exceed the internal data buffer size. Both will lead into hard to find bugs if the passed data buffer size is outside the limits according to the CAN standard. This fix intends to solve this by limiting the input data size to 8 bytes.

@mergify mergify bot added needs: work and removed needs: CI labels Feb 17, 2020
@0xc0170 0xc0170 changed the title Fix for issue #12311 CAN: fix length calculation (fixes #12311) Feb 17, 2020
@mbed-ci
Copy link

mbed-ci commented Feb 17, 2020

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

The two types of the CANMessage constructor accepting a data buffer have two issues. First, they limit the input buffer size to the 4 least significant bits of the passed length even though a CAN message cannot have more than 8 bytes of payload. Second, the used data length in the following memcpy() uses the initially passed data length which may exceed the internal data buffer size. Both will lead into hard to find bugs if the passed data buffer size is outside the limits according to the CAN standard. This fix intends to solve this by limiting the input data size to 8 bytes.
@mergify mergify bot dismissed 0xc0170’s stale review February 17, 2020 21:04

Pull request has been modified.

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 18, 2020

CI restarted

@mbed-ci
Copy link

mbed-ci commented Feb 18, 2020

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts

@0xc0170 0xc0170 merged commit 6a79d0e into ARMmbed:master Feb 19, 2020
@mergify mergify bot removed the ready for merge label Feb 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants