Fix buffer overflow in _cbor_value_copy_string

[jimparis]

The function is documented to only null-terminate when the buffer is big enough to allow it. Both upstream intel/tinycbor and mynewt's version do this correctly.

See zephyrproject-rtos/zephyr#19629 for details.

[mbolivar]

Both upstream intel/tinycbor and mynewt's version do this correctly.

cc @nashif

@jimparis if that is the case, why not just update this repository to a revision from intel/tinycbor which has the fix?

[jimparis]

Because this repo has diverged significantly from intel/tinycbor due to PR #1: 26 files changed, 856 insertions(+), 222 deletions(-). It was that PR that introduced this bug.

[mbolivar]

It looks like that PR aligned this repository with the mynewt version of the library. Since you say mynewt's version also contains the fix, can we do another update to the latest mynewt code?

I'm concerned about this project diverging so significantly from its upstreams that it becomes a hard fork.

[nvlsianpu]

I would merge this ASAP. We can glue the history (git merge --allow-unrelated-histories)) after that. This merge is urgent IMO.

[jimparis]

It looks like that PR aligned this repository with the mynewt version of the library. Since you say mynewt's version also contains the fix, can we do another update to the latest mynewt code?

The fix is not in mynewt because they never had the bug. This bug was created by the huge code changes in PR #1, most of which appear to have been brand new Zephyr-specific code.

I'm concerned about this project diverging so significantly from its upstreams that it becomes a hard fork.

It already has. The commit message for PR #1 has 18 bullet points, and only the first one menitioned syncing with upstream mynewt tinycbor (which version, I have no idea -- I have a hard time finding matching code). This repository is vastly different from both intel/tinycbor and the copy in apache/mynewt-core.

[mbolivar]

I would merge this ASAP. We can glue the history (git merge --allow-unrelated-histories)) after that. This merge is urgent IMO.

@nvlsianpu it's a small fix and I'm fine with merging it, but it looks like we have a big problem with this project based on information from @jimparis:

This repository is vastly different from both intel/tinycbor and the copy in apache/mynewt-core.

How are we going to fix this? We shouldn't have modules that diverge this much from upstream IMO. Maybe PR #1 should never have been merged?

@nashif @carlescufi thoughts on what to do?

[jimparis]

I found some more history behind these changes in these PRs:
zephyrproject-rtos/zephyr#5912
intel#83 (not merged)

Original commit to zephyr with the buffer overflow introduced was this:
zephyrproject-rtos/zephyr@bc0a91d

Which was from this commit
intel@ece8b46 (not merged)

FYI someone else will have to take over untangling this -- I will be basically unavailable from around Oct 10 - Nov 20.

[mbolivar]

git merge --allow-unrelated-histories

This also isn't just a clerical matter of "gluing" histories together. It's about the code base not accumulating technical debt and forcing our users to find and fix bugs that should have been managed upstream.

[carlescufi]

This repository is vastly different from both intel/tinycbor and the copy in apache/mynewt-core.

Maybe we should align with the copy in mynewt then. The reason we took the changes that diverged from upstream TinyCBOR was for this to work correctly with mcumgr, which is a cross-platform library that both Zephyr and Mynewt use as a DFU protocol.
@nvlsianpu can you please check if we could align with Mynewt's TinyCBOR so that at least we don't have 3 different versions?

[nvlsianpu]

^^Will check
@rakons What your opinion of aligning this code to mynewt copy, or intel?

[de-nordic]

I have looked into the fix and it should be fine, with one remark
if we fill buffer to the edge, with no 0 termination as result, this is risky for null-terminated buffer expecting functions, none of such uses as far as I can tell for now, and callers relay on length returned by the fixed function anyway.

I agree on the need to align with one of upstreams.

[rakons]

@nvlsianpu I have no strong opinion. I have just used it once. I agree that we should rather stick to one version and not create our own.

[nvlsianpu]

There was a trial to push the patche required by the zephyr mcumgr fork (both were libraries in the zephyr's directories in those-days) library to the intel fork, but without success as PR stall.

[the zephyr tinycbor] was modified in order to made it OS agnostic (as far I can remember).
I believe that it will be easier to synchronize witch the mynewt fork as it was real ancestor of the zephyr tinycbor fork. Also it make sense as tinycbor in mynewt is used by mcumgr as well, so it is
checked at work.
cc @vrahane

[de-nordic]

Lets get it done. The fix is fine for now, until we sync/align with newt or intel version.