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

VxWorks OSAL uses potentially unsafe "strcpy" operations #195

Closed
skliper opened this issue Sep 30, 2019 · 7 comments · Fixed by #443
Closed

VxWorks OSAL uses potentially unsafe "strcpy" operations #195

skliper opened this issue Sep 30, 2019 · 7 comments · Fixed by #443
Assignees
Labels
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Sep 30, 2019

In at several locations the VxWorks OSAL is using strcat() and strcpy() functions to copy strings between string buffers that are sized using different macros.

At least one of the sizes in play, the OS_MAX_PATH_LEN comes from the user-configurable "osconfig.h" file. Other sizes, such as OS_FS_PHYS_NAME_LEN are specified in the local headers and are not user-configurable.

In some functions, such as OS_mkfs (but not limited to this), a local buffer of size OS_MAX_PATH_LEN is copied into a global buffer of size OS_FS_PHYS_NAME_LEN.

However, because the OS_MAX_PATH_LEN is configurable via the osconfig.h file, it is not guaranteed that OS_MAX_PATH_LEN is less than or equal to OS_FS_PHYS_NAME_LEN.

@skliper skliper self-assigned this Sep 30, 2019
@skliper skliper added the bug label Sep 30, 2019
@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Imported from trac issue 172. Created by jphickey on 2016-07-26T17:07:26, last modified: 2019-08-14T14:11:46

@skliper
Copy link
Contributor Author

skliper commented Sep 30, 2019

Trac comment by jphickey on 2019-03-26 08:52:42:

This can be added to the pool of "stuff that gets fixed by migrating to NG architecture". After #231 gets merged this should be closed.

@skliper skliper assigned jphickey and unassigned skliper Sep 30, 2019
@skliper skliper added this to the 5.1.0 milestone Oct 22, 2019
@skliper
Copy link
Contributor Author

skliper commented Oct 22, 2019

Just needs close review to confirm all cases were addressed.

@skliper
Copy link
Contributor Author

skliper commented Jan 2, 2020

Only remaining concern:

os/shared/osapi-printf.c: strcpy(console->device_name, OS_PRINTF_CONSOLE_NAME);

Could be defined in osconfig.h, but no check on size seen (quick inspection).

@skliper
Copy link
Contributor Author

skliper commented Apr 20, 2020

Want me to reassign this one? @jphickey

@jphickey
Copy link
Contributor

No, I'll just change that last case to an strncpy instead, as part of the other refactoring I am doing for #285. Any change made separately will likely cause merge woes.

Note that the symbol OS_PRINTF_CONSOLE_NAME would be defined in osconfig.h, which the same file as OS_MAX_API_NAME (the actual length limit) is defined. But it doesn't hurt to use strncpy instead of strcpy here just in case the user misconfigures, and its a trivial change in the code.

@skliper
Copy link
Contributor Author

skliper commented Jun 5, 2020

Closed in #443

@skliper skliper closed this as completed Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants