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

x509.c & x509_crt.c do not guard use of some POSIX <sys/*> and other headers #992

Open
eschaton opened this Issue Jul 2, 2017 · 5 comments

Comments

Projects
None yet
5 participants
@eschaton

eschaton commented Jul 2, 2017

In library/x509.c and library/x509_crt.c there are includes of <sys/types.h>, <sys/stat.h>, and <dirent.h> that are only guarded by MBEDTLS_FS_IO and not by a HAVE_… macro. This means that the file won’t compile on a system that might not have POSIX headers but where a developer might still want to access the filesystem via C Standard I/O. (An example of such a system would be Mac OS 9.)

@eschaton eschaton changed the title from library/x509.c does not guard use of some POSIX <sys/*> and other headers to x509.c & x509_crt.c do not guard use of some POSIX <sys/*> and other headers Jul 2, 2017

@RonEld RonEld added the tracking label Jul 3, 2017

@RonEld

This comment has been minimized.

Contributor

RonEld commented Jul 3, 2017

Hi @eschaton Thanks for raising this!
We will look into it

@ciarmcom ciarmcom added the mirrored label Jul 3, 2017

@ciarmcom

This comment has been minimized.

Member

ciarmcom commented Jul 3, 2017

ARM Internal Ref: IOTSSL-1504

@sbutcher-arm sbutcher-arm added the bug label Jul 18, 2017

mrpippy added a commit to bslabs/mbedtls that referenced this issue Sep 19, 2017

@mrpippy mrpippy referenced this issue Apr 7, 2018

Merged

x509.c: Remove unused includes #1563

0 of 4 tasks complete
@mrpippy

This comment has been minimized.

Contributor

mrpippy commented Apr 7, 2018

I looked into this further. The includes in x509.c are not needed, and are probably just left over from before x509 was split into multiple files. I opened #1563 to remove those includes.

In x509_crt.c, mbedtls_x509_crt_parse_path() uses readdir()and stat() on non-Win32 to iterate through the given directory. This appears to be the only file I/O in the library that doesn't/can't use standard C file I/O functions.

I'm not sure of the best way to resolve this. readdir() and stat() are used on all non-Win32 platforms (not just those that identify as UNIX) that define MBEDTLS_FS_IO, but I guess it hasn't been a big problem so far. For example, even mbed-os appears to implement readdir() and stat().

It would be possible to create an MBEDTLS_HAVE_READDIR or similar define. But, what should happen to mbedtls_x509_crt_parse_path() when this is false? ifdef the function out entirely? Or have it throw a compile-time warning and runtime error code? It is part of the external API, i.e. cURL does call it.

In searching, I did find two other reports of this function being problematic because of how platform-specific it is, on both bare-metal ARM and Windows UWP:
https://devzone.nordicsemi.com/f/nordic-q-a/17815/error-error-dirent-h-not-supported
https://jira.iotivity.org/browse/IOT-1449

@eschaton

This comment has been minimized.

eschaton commented Apr 7, 2018

My personal preference would be for indirecting through other functions to perform directory iteration so that something like MBEDTLS_HAVE_READDIR would only be used in one place, rather than (potentially) several, so platforms without readdir() could implement their own iteration.

For example, a bare-metal platform could have one or several certificate stores in flash RAM while a platform like classic Mac OS could use its own iteration to do something equivalent to readdir() and stat() without necessarily directly emulating those APIs.

mrpippy added a commit to bslabs/mbedtls that referenced this issue Apr 7, 2018

MacOS: Re-enable MBEDTLS_FS_IO, and stub out mbedtls_x509_crt_parse_p…
…ath() which uses POSIX readdir(). Tracking issue in upstream issue ARMmbed#992
@mrpippy

This comment has been minimized.

Contributor

mrpippy commented Apr 7, 2018

True, I think that would be a good solution. Similar to TIMING_ALT, have a define that triggers usage of a platform-provided function that iterates through a given directory and calls a provided callback for each file.
Moving the Unix/Win32 implementation out of x509_crt.c is probably a good idea anyway.

sbutcher-arm added a commit to bslabs/mbedtls that referenced this issue Jul 2, 2018

sbutcher-arm added a commit to sbutcher-arm/mbedtls that referenced this issue Jul 2, 2018

sbutcher-arm added a commit to sbutcher-arm/mbedtls that referenced this issue Jul 2, 2018

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