Openssl #93

jforissier · 2014-10-29T09:57:42Z

This patch series implements the internal layer that allows the use of cryptographic libraries other than the default LibTomCrypt. It adds an OpenSSL implementation.

The first patch: Add crypto provider internal API is basically what was reviewed in PR Add crypto provider internal API. #35. It makes a clean separation between the crypto services (tee_svc_cryp.c) and the LibTomCrypt implementation.
The second patch: Import libcrypto source code from OpenSSL 1.0.1i brings in some OpenSSL code under core/lib/openssl. The code is committed unmodified, and is not built at this stage This commit can serve as a reference point when merging future versions of OpenSSL.
The third patch: OpenSSL: AES-XTS: allow multiple calls... is an enhancement needed for proper TEE_ALG_AES_XTS operations. It deals with encryption and decryption of partial buffers.
The fourth patch: libutils: add functions needed by OpenSSL... adds some standard functions that are required to build OpenSSL.
The fifth patch: Add support for $(lib-use-ld) and $(lib-ldflags) is a small addition to the OP-TEE build system. It is used in the next patch to avoid adding unused functions to the OpenSSL library (and thus reduce the final size of the TEE binary).
Finally, the sixth patch implements the OpenSSL crypto provider.

jenswi-linaro · 2014-10-31T06:42:59Z

Looks good as far as I can tell. I'd like to go through it once more after all the new commits has been squashed in where they belong.

pascal-brand38 · 2014-10-31T07:50:27Z

Same for me.
I'd also like to run full-testing with both LTC and openssl, on Cannes.

I've also asked Hervé Sibert to check about the licenses of the new added
files (openssl / newlib). I do not expect anything wrong.

On 31 October 2014 07:42, Jens Wiklander notifications@github.com wrote:

Looks good as far as I can tell. I'd like to go through it once more after
all the new commits has been squashed in where they belong.

—
Reply to this email directly or view it on GitHub
#93 (comment).

jforissier · 2014-10-31T07:58:56Z

OK, thanks to you both for the review. I am going to rebase the branch, merging the review patches with the previous ones where they belong.
Pascal, do you want me to add some OpenSSL build to Travis? One platform should be enough since this is basically common code.

pascal-brand38 · 2014-10-31T08:02:41Z

Yes, testing openssl compilation is good.
One existing platform is sufficient I guess.

On 31 October 2014 08:58, Jérôme Forissier notifications@github.com wrote:

OK, thanks to you both for the review. I am going to rebase the branch,
merging the review patches with the previous ones where they belong.
Pascal, do you want me to add some OpenSSL build to Travis? One platform
should be enough since this is basically common code.

—
Reply to this email directly or view it on GitHub
#93 (comment).

hsibert · 2014-10-31T12:02:12Z

HI,

I have looked at the license. It's almost fine, but I see some problems. It’s mainly BSD but it adds some conditions, like “Redistributions of any form whatsoever must retain the following
acknowledgment:"This product includes software developed by the OpenSSL Project
for use in the OpenSSL Toolkit (http://www.openssl.org/)"

We should not get forced into that because of SW that in fact is not used by default, so I would much prefer that OP-TEE does not have to include OpenSSL SW.

Is there a way to keep OP-TEE free from OpenSSL code? (it's not really good advertizing for security SW to include OpenSSL at this time :))

People who would want to replace LTC with OpenSSL and distribute OP-TEE with it would then have to fulfill the OpenSSL license but we, on the OP-TEE repository, would not. Moreover, including OpenSSL in OP-TEE would also raise the question of which OpenSSL version to incorporate, how to manage evolutions (as OpenSSL is evolving frequently), and I don't think we want to do that. And the option of including a given version that probably has many vulnerabilities and leaving it as is is not good either.

All in all we should keep only the wrapper layers and the additional OpenSSL-supporting functions in OP-TEE.

jbech-linaro · 2014-10-31T12:43:30Z

I guess one of the problems we have here is that we would like to show
that this is actually working. As it turned out it wasn't as simple as
just add the wrapper layer and OpenSSL supporting functions to get this
to work (demonstrable). There were all OpenSSL intra-dependencies that
Jerome had to fix to actually be able to compile and run this.

The main idea with this work package was to loosen up the hard
coupling with LTC, so it should be easy to use other software libraries
or even use hardware. That goal has been fulfilled already in the first
of the six patches.

Hervé's comment about keeping up-to-date with latest OpenSSL is indeed a
problem (however not a requirement for us), likewise the comments about
including OpenSSL directly in OP-TEE. Couldn't we just merge the first
patch and then put patch two to six in a separate branch in another
repository? Not optimal, but maybe a good trade-off? In our
documentation we could simply refer to this other branch and the
potential users would have a very smooth journey when enabling and using
OpenSSL instead of LTC? Using Travis magic we could still test this.

Regards,
Joakim B

jforissier · 2014-10-31T14:06:57Z

On Fri, Oct 31, 2014 at 1:02 PM, Herve Sibert notifications@github.com
wrote:

HI,

I have looked at the license. It's almost fine, but I see some problems.
It’s mainly BSD but it adds some conditions, like “Redistributions of any
form whatsoever must retain the following
acknowledgment:"This product includes software developed by the OpenSSL
Project
for use in the OpenSSL Toolkit (http://www.openssl.org/)"

We should not get forced into that because of SW that in fact is not used
by default, so I would much prefer that OP-TEE does not have to include
OpenSSL SW.

IANAL, but I think it would be enough to keep the LICENSE file in the
openssl/ directory as well as the copyright/license headers in the source
files.

Is there a way to keep OP-TEE free from OpenSSL code? (it's not really
good advertizing for security SW to include OpenSSL at this time :))

As Joakim said, just merge only the first commit which adds the internal
interface and reworks the LTC integration.
The OpenSSL stuff might not be useful in its current state, but it could
facilitate the implementation in OP-TEE of additional features, such as new
algorithms and hardware acceleration. In any case it was really helpful to
develop and validate the interface, which was really the number one
objective.

People who would want to replace LTC with OpenSSL and distribute OP-TEE
with it would then have to fulfill the OpenSSL license but we, on the
OP-TEE repository, would not.

Yes, but again my feeling is that we could integrate the openssl/ directory
without any other obligation.

Moreover, including OpenSSL in OP-TEE would also raise the question of
which OpenSSL version to incorporate, how to manage evolutions (as OpenSSL
is evolving frequently), and I don't think we want to do that. And the
option of including a given version that probably has many vulnerabilities
and leaving it as is is not good either.

Same concern applies to LTC ;-) And BTW, we're just using the crypto
library of OpenSSL, not the whole SSL/TLS protocol and certificate
processing stuff (which is more likely to bear the vulnerabilities). Still,
this is a valid point.

All in all we should keep only the wrapper layers and the additional
OpenSSL-supporting functions in OP-TEE.

This sounds reasonable. For the time being, I am rebasing/squashing some
review changes to finalize the review. You can then pick the commits you
want for OP-TEE master.
Then as Joakim suggested, if OP-TEE does not include OSSL by default, we
can still refer potential users to the code as needed. There is no need to
create a new repo for this, since the openssl branch of my repo (and this
PR) can serve that purpose.

—
Reply to this email directly or view it on GitHub
#93 (comment).

jforissier · 2014-10-31T15:03:13Z

New, consolidated patch series pushed. All review comments have hopefully been addressed.
Jens, Pascal, would you please post your Reviewed-by/Tested-by tags to each individual commit, as applicable?

hsibert · 2014-11-01T00:08:07Z

Hi,

I think Joakim's proposal to merge only the first patch is good as it allows to keep the main branch free of OpenSSL stuff.
A dedicated branch to show the integration of OpenSSL is welcome. It will only bear the license file. However, anyone who uses the branch for redistribution and production will have to fulfill the OpenSSL license, meaning, for instance, that for a final product the final manufacturer will have to print the acknowledgements.
So if everyone agrees, then it's ok to:

merge the first patch in the main branch,
create an openssl branch that complements the main with all the needed openSSL stuff (including the license)

By doing so, the openSSL branch will stay a proof-of-concept that we do not have to maintain and that can help those who are willing to re-integrate the latest OpenSSL version (or the latest revision of the same version).

In the documentation for the main branch, we should just mention the existence of the openssl branch that integrates a given version of the openssl crypto lib instead of LTC, without further details. The details should be in the branch's documentation files so there is no doubt with the license terms.

pascal-brand38 · 2014-11-03T07:27:12Z

+1.

What about

jforissier@befcf6a
jforissier@320670b

It could be good to have them in the master branch.

Regards,
Pascal.

On 1 November 2014 01:08, Herve Sibert notifications@github.com wrote:

Hi,

I think Joakim's proposal to merge only the first patch is good as it
allows to keep the main branch free of OpenSSL stuff.
A dedicated branch to show the integration of OpenSSL is welcome. It will
only bear the license file. However, anyone who uses the branch for
redistribution and production will have to fulfill the OpenSSL license,
meaning, for instance, that for a final product the final manufacturer will
have to print the acknowledgements.
So if everyone agrees, then it's ok to:

merge the first patch in the main branch,

create an openssl branch that complements the main with all the
needed openSSL stuff (including the license)

By doing so, the openSSL branch will stay a proof-of-concept that we do
not have to maintain and that can help those who are willing to
re-integrate the latest OpenSSL version (or the latest revision of the same
version).

In the documentation for the main branch, we should just mention the
existence of the openssl branch that integrates a given version of the
openssl crypto lib instead of LTC, without further details. The details
should be in the branch's documentation files so there is no doubt with the
license terms.

—
Reply to this email directly or view it on GitHub
#93 (comment).

jforissier · 2014-11-03T07:40:49Z

On Mon, Nov 3, 2014 at 8:27 AM, Pascal Brand (dev) <notifications@github.com

wrote:

+1.

What about

jforissier@befcf6a

jforissier@320670b

It could be good to have them in the master branch.

Agreed. The first one for instance may be helpful soon, when we make
algorithms selectable at compile time. Like I did for OpenSSL, by using
partial linking and --gc-sections, we can avoid incorporating unused LTC
functions in the final binary.

hsibert · 2014-11-03T08:29:57Z

+1, yes let’s integrate these patches as well.

Thank you Jérôme and Pascal ☺

From: Jérôme Forissier [mailto:notifications@github.com]
Sent: lundi 3 novembre 2014 08:41
To: OP-TEE/optee_os
Cc: Herve SIBERT
Subject: Re: [optee_os] Openssl (#93)

On Mon, Nov 3, 2014 at 8:27 AM, Pascal Brand (dev) <notifications@github.com
mailto:notifications@github.com%0b> wrote:

+1.

What about

jforissier@befcf6a

jforissier@320670b

It could be good to have them in the master branch.

Agreed. The first one for instance may be helpful soon, when we make
algorithms selectable at compile time. Like I did for OpenSSL, by using
partial linking and --gc-sections, we can avoid incorporating unused LTC
functions in the final binary.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/93#issuecomment-61447098.

jforissier · 2014-11-04T09:48:13Z

Patch series updated as follows:

Added Reviewed-by tag to e462836 (crypto internal API), a39fcaf (lib-use-ld), a6da6c0 (cflags-lib-y)
Addressed comments made by Jens on 26f3fce (libutils, kernel...), changes squashed into 05c0206.

$ md5sum openssl-1.0.1i.tar.gz c8dc151a671b9b92ff3e4c118b174972 openssl-1.0.1i.tar.gz Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>

This commit modifies CRYPTO_xts128_encrypt() so that tweak information is saved in the context for the next call. As a result, EVP_EncryptUpdate() or EVP_DecryptUpdate() can be called multiple times with partial buffers.

Prepare for building OpenSSL libcrypto. 1. Add the following to libutils: - isdigit(), isspace(), isalpha(), isalnum(), isxdigit(), isupper(), islower(), toupper(), tolower(): basic ASCII-only implementation - atoi(), sscanf() (stub!), strtoul(), strncmp(), strcpy(), strncpy(), strchr(), strcat() 2. Add abort() and time() to core/kernel. Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>

Set the WITH_CRYPTO build option to 'openssl' to use the OpenSSL libcrypto library for cryptographic services. When WITH_CRYPTO is not set, or set to 'tomcrypt', LibTomCrypt is used. Usage example: $ make CROSS_COMPILE=arm-linux-gnueabihf- \ PLATFORM=vexpress PLATFORM_FLAVOR=fvp \ WITH_CRYPTO=openssl Tested with xtest (e7cda93) on Foundation_v8 (PLATFORM_FLAVOR=fvp) and QEMU (PLATFORM_FLAVOR=qemu_virt). Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>

jforissier · 2014-11-10T16:45:18Z

Rebased on 3d34e12 (i.e., what was merged into master). So here remains only the OpenSSL-specific stuff.

ghost · 2014-11-17T07:37:54Z

Has been pushed on the branch

poc/openssl_cryptolib

jforissier force-pushed the openssl branch from 5f923ae to c105d6a Compare October 31, 2014 14:56

jforissier force-pushed the openssl branch from c105d6a to 1554794 Compare November 4, 2014 09:39

jforissier added 5 commits November 10, 2014 17:25

Import libcrypto source code from OpenSSL 1.0.1i

c2cdf8a

$ md5sum openssl-1.0.1i.tar.gz c8dc151a671b9b92ff3e4c118b174972 openssl-1.0.1i.tar.gz Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>

openssl: AES-XTS: allow updates with partial buffers

f546645

This commit modifies CRYPTO_xts128_encrypt() so that tweak information is saved in the context for the next call. As a result, EVP_EncryptUpdate() or EVP_DecryptUpdate() can be called multiple times with partial buffers.

Travis: add build command with OpenSSL crypto enabled

b9acebb

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>

jforissier force-pushed the openssl branch from 1554794 to b9acebb Compare November 10, 2014 16:42

ghost closed this Nov 17, 2014

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Openssl #93

Openssl #93

jforissier commented Oct 29, 2014

jenswi-linaro commented Oct 31, 2014

pascal-brand38 commented Oct 31, 2014

jforissier commented Oct 31, 2014

pascal-brand38 commented Oct 31, 2014

hsibert commented Oct 31, 2014

jbech-linaro commented Oct 31, 2014

jforissier commented Oct 31, 2014

jforissier commented Oct 31, 2014

hsibert commented Nov 1, 2014

pascal-brand38 commented Nov 3, 2014

jforissier commented Nov 3, 2014

hsibert commented Nov 3, 2014

jforissier commented Nov 4, 2014

jforissier commented Nov 10, 2014

ghost commented Nov 17, 2014

Openssl #93

Openssl #93

Conversation

jforissier commented Oct 29, 2014

jenswi-linaro commented Oct 31, 2014

pascal-brand38 commented Oct 31, 2014

jforissier commented Oct 31, 2014

pascal-brand38 commented Oct 31, 2014

hsibert commented Oct 31, 2014

jbech-linaro commented Oct 31, 2014

jforissier commented Oct 31, 2014

jforissier commented Oct 31, 2014

hsibert commented Nov 1, 2014

pascal-brand38 commented Nov 3, 2014

jforissier commented Nov 3, 2014

hsibert commented Nov 3, 2014

jforissier commented Nov 4, 2014

jforissier commented Nov 10, 2014

ghost commented Nov 17, 2014