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

Fixes different off by ones #1663

Merged
merged 1 commit into from Jul 20, 2018

Conversation

Projects
None yet
5 participants
@catenacyber
Contributor

catenacyber commented May 30, 2018

Description

Fixes different off by one overreads

Status

READY

Requires Backporting

I guess so
Which branch? do not know

Migrations

NO

Additional comments

These do not seem security issues as the orignal buffer is overgrown.
But they are bad coding practices, and the checks are already performed most of the times.
Some previously similar bugs from the ones reported by mail have already been corrected like
740b218

Todos

  • Tests
  • Documentation
  • Changelog updated
  • Backported

Steps to test or reproduce

Outline the steps to test or reproduce the PR here.

@mpg

mpg approved these changes Jun 4, 2018

Looks good to me.

@mpg

This comment has been minimized.

Contributor

mpg commented Jun 4, 2018

@catenacyber Thanks for your contribution! The fixes look good, however:

  • a compiler warning was caught by the CI and needs to be fixed:
/home/travis/build/ARMmbed/mbedtls/library/ssl_cli.c: In function ‘ssl_parse_supported_point_formats_ext’:
/home/travis/build/ARMmbed/mbedtls/library/ssl_cli.c:1250:32: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
     if( len == 0 || buf[0] + 1 != len )
                                ^
cc1: all warnings being treated as errors
  • Feel free to include a ChangeLog entry crediting yourself for those changes. (Unlike other projects, we add ChangeLog entries withing each PR rather than after merging.)
@mpg

This comment has been minimized.

Contributor

mpg commented Jun 4, 2018

Requires Backporting
I guess so
Which branch? do not know

This should be backported to 2.7 and 2.1 (our current maintained branches). This is done by creating new branches based on those with the backports, and then one new PR for each backport, with the same title as this one and a "Backport 2.x:" prefix, and "backport of #1663" as the PR cover text.

@mpg mpg added the needs: backports label Jun 4, 2018

@catenacyber

This comment has been minimized.

Contributor

catenacyber commented Jun 4, 2018

a compiler warning was caught by the CI and needs to be fixed

I am pushing an update

Feel free to include a ChangeLog entry crediting yourself for those changes

Ok, but how so ?
Should I just edit the ChangLog file ?
If so, should I do my pull request on development branch rather than master branch ?

This should be backported to 2.7 and 2.1

Ok, I will do it when build is green

@mpg

This comment has been minimized.

Contributor

mpg commented Jun 5, 2018

Thanks for updating the PR. Unfortunately it looks like the CI is seeing some other issues now.

Should I just edit the ChangeLog file ?

Yes, that's the way it's done. You'll need to create section for the next release as = mbed TLS x.x.x branch released xxxx-xx-xx and then place your entry in the appropriate sub-section (in that case I'm not sure which one is the best, perhaps Changes - as there is no functional or security impact).

If so, should I do my pull request on development branch rather than master branch ?

From the github interface, it looks like that's what you're already doing, and this is indeed correct. (Our main branch is called development, and master only points to the latest stable release.)

@catenacyber catenacyber force-pushed the catenacyber:offbyone branch from b924937 to 2d3cb9d Jun 5, 2018

@catenacyber

This comment has been minimized.

Contributor

catenacyber commented Jun 5, 2018

@mpg I fixed one more warning, but now I do not understand where Travis is failing. Do you ?

@mpg

This comment has been minimized.

Contributor

mpg commented Jun 5, 2018

I think the Travis failures are unrelated to your PR and will be fixed by #1683 - once that PR is merged, I suggest you rebase on development in order to get a green light from Travis, and in the meantime you can just ignore that error.

@mpg

mpg approved these changes Jun 5, 2018

Looks good to me. Thanks for fixing the warnings.

@catenacyber catenacyber force-pushed the catenacyber:offbyone branch from 2d3cb9d to 6b2281d Jun 5, 2018

@catenacyber catenacyber force-pushed the catenacyber:offbyone branch from 6b2281d to 747fd53 Jun 5, 2018

@catenacyber

This comment has been minimized.

Contributor

catenacyber commented Jun 5, 2018

@mpg ok looks green
Should I wait for @hanno-arm review ?
Or should I now create the backports ?

@mpg

This comment has been minimized.

Contributor

mpg commented Jun 6, 2018

I think it's your choice, you can either go ahead with the backports now, with a slight risk of having to backports changes again if Hanno requests any, or wait for his review to avoid that risk, but then you'll be waiting. (If it helps, I think most people in the team tend to go ahead with backports once we've received one approval, but that's still a personal choice.)

@sbutcher-arm sbutcher-arm requested review from sbutcher-arm and removed request for hanno-arm Jun 8, 2018

@mpg mpg self-assigned this Jun 20, 2018

@sbutcher-arm

Reviewed, tested and approved.

Tested with full config (minus NV Seed and Memory Buffer features) on Ubuntu, with test suites, ssl-opt.sh and compat.sh.

@mpg

This comment has been minimized.

Contributor

mpg commented Jul 3, 2018

@catenacyber This PR is now fully approved. Do you want to take care of creating the backports? Either way, let us know, so that we can work on the backports ourselves if you don't. Thanks!

@mpg mpg removed the ready for merge label Jul 3, 2018

@mpg

This comment has been minimized.

Contributor

mpg commented Jul 3, 2018

(Removing "ready for merge" label as we're waiting for backports.)

@catenacyber

This comment has been minimized.

Contributor

catenacyber commented Jul 9, 2018

Here are the backports. I hope I pushed them correctly

@mpg

This comment has been minimized.

Contributor

mpg commented Jul 9, 2018

@catenacyber Thanks for the backports! They look fine to me.

@mpg mpg added the ready for merge label Jul 11, 2018

@sbutcher-arm sbutcher-arm merged commit 747fd53 into ARMmbed:development Jul 20, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

sbutcher-arm added a commit that referenced this pull request Jul 20, 2018

Fix ChangeLog entry for issue #1663
The ChangeLog entry was under the wrong version, and under Changes, not
BugFixes.

sbutcher-arm added a commit that referenced this pull request Jul 20, 2018

Fix ChangeLog entry for issue #1663
The ChangeLog entry was under the wrong version, and under Changes, not
Bug Fixes.

sbutcher-arm added a commit that referenced this pull request Jul 20, 2018

Fix ChangeLog entry for issue #1663
The ChangeLog entry was under the wrong version, and under Changes, not
Bug Fixes.
@@ -2711,7 +2711,7 @@ static int ssl_parse_certificate_request( mbedtls_ssl_context *ssl )
* therefore the buffer length at this point must be greater than that
* regardless of the actual code path.
*/
if( ssl->in_hslen <= mbedtls_ssl_hs_hdr_len( ssl ) + 2 + n )
if( ssl->in_hslen <= mbedtls_ssl_hs_hdr_len( ssl ) + 3 + n )

This comment has been minimized.

@hanno-arm

hanno-arm Aug 2, 2018

Contributor

@catenacyber @mpg @sbutcher-arm What is the justification for this? To me it seems that the previous check was correct.

This comment has been minimized.

@catenacyber

catenacyber Aug 3, 2018

Contributor

Indeed, maybe this was about the problem with

            MBEDTLS_SSL_DEBUG_MSG( 3, ( "Supported Signature Algorithm found: %d"
                                        ",%d", sig_alg[i], sig_alg[i + 1]  ) );

fixed by bc231cc

This comment has been minimized.

@mpg

mpg Aug 10, 2018

Contributor

@hanno-arm It seems to me that if ssl->in_hslen == mbedtls_ssl_hs_hdr_len( ssl ) + 2 + n then neither access to buf[mbedtls_ssl_hs_hdr_len( ssl ) + 2 + n] on line 2727 below (in the "TLS 1.2" branch) nor the same access on line 2768 below (next if TLS version < 1.2) are properly guarded, so the previous test was not correct and the new one is.

Or perhaps I just didn't have enough tea this morning?

This comment has been minimized.

@hanno-arm

hanno-arm Aug 10, 2018

Contributor

@mpg My point is that the code fails even if ssl->in_hslen == mbedtls_ssl_hs_hdr_len( ssl ) + 3 + n which seems unnecessary.

This comment has been minimized.

@hanno-arm

hanno-arm Aug 10, 2018

Contributor

If that's the case and we're below TLS 1.2, and dn_len == 0, then we might fail to parse a valid CertificateRequest message. This case is explicitly allowed in the TLS 1.1 RFC 4346, Section-7.4.4:

      certificate_authorities
         A list of the distinguished names of acceptable certificate
         authorities.  These distinguished names may specify a desired
         distinguished name for a root CA or for a subordinate CA; thus,
         this message can be used to describe both known roots and a
         desired authorization space.  If the certificate_authorities
         list is empty then the client MAY send any certificate of the
         appropriate ClientCertificateType, unless there is some
         external arrangement to the contrary.

This comment has been minimized.

@mpg

mpg Aug 10, 2018

Contributor

Ok, so that was the "not enough tea" branch: I missed that the test if <= not <. Indeed please disregard my previous comment, I agree with your analysis.

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