invec/outvec checks TOCTOU
Open, Needs TriagePublic

Description

secure_fw/core/ipc/tfm_svcalls.c tfm_svcall_psa_call

invec/outvec pointer and sizes validations must be moved to` tfm_spm_create_msg` and validated after they are copied to a message to prevent TOCTOU attack.

in_vec is a pointer in the caller memory, thus accessible and modifiable by the caller.

all the validations performed on individual invec/outvec may no longer be valid after a context switch (e.g. NSPE interrupt)

alzix created this task.Jan 27 2019, 8:56 PM

Thanks, Alexander, currently we copy into a local buffer and then moved to the message instead of moving to message directly due to message buffer allocation limitation (https://developer.trustedfirmware.org/T195)

alzix added a comment.Jan 28 2019, 7:54 AM

from what i see invecs are not copied. Perhaps i'm looking at outdated sources? https://review.trustedfirmware.org/c/trusted-firmware-m/+/468/1/secure_fw/core/ipc/tfm_svcalls.c#131

in_vec = (psa_invec *)((psa_invec *)args[1])->base;
in_num = ((psa_invec *)args[1])->len;
out_vec = ((psa_outvec *)args[2])->base;
out_num = ((psa_outvec *)args[2])->len;

Hi Alex,
This line is for extracting NSPE invec/outvec from invec due to parameters number limitation. The anti-TOCTOU is added in line: 177 now working for secure partitions.

The line you pointed of missing TOCTOU for NSPE parameter is correct -- This NS handling part will be changed soon due to the existing implementation is a bit workaround. After the implementation of aligning veneer and secure partition SVCalls for psa_call, we could remove the ns_caller == true code blocks.

alzix added a comment.Jan 29 2019, 8:50 AM

the spec specifies that caller passes a pointer array of psa_invec. The array is allocated in caller memory, thus is modifiable by a caller at any time.
SPM must first copy each individual psa_invec to SPM own memory, and only then verify accessibility of each individual range.

Consider use case where you have finished your iteration on psa_invec and tested all the ranges., Then there is a context switch to NSPE due to NS-Irq. Offending NSPE can modify the in/out vector array data and return to SPE. Then SPM will copy bad psa_invec contents to the tfm_spm_create_msg.

The rule of thumb for preventing TOCTOU is to either read once or copy first and test later.

In addition you cannot assumed that SG was called from TF-M veneer. Offending NSPE code can call SG directly bypassing TF-M veneers. Thus all pointers themselves received from NSPE must be treated in a similar way.
The local struct allocated on NSPE caller stack and used to pack the arguments is both modifiable and the pointer itself can point to arbitrary memory, so you must also check accessibility of the struct received by pointer from TF-M veneers.

wmnt added a comment.Jan 29 2019, 8:53 AM

Alex,

Please note that secure SVC is running on highest priority. Execution of this code cannot be pre-empted by either NSPE or any external secure interrupt. This is essentially a critical section.

alzix added a comment.Jan 29 2019, 8:59 AM

Miklos,
As always you are 100% right. I just wanted to provide simplistic example. Perhaps i've oversimplified :).
While you assumption is true for single core systems, it breaks on asymmetrical multi-core system where SPM is running on one core and NSPE on other.
TF-M is not supporting such a targets right now, but we are now working on a port for such a target.
I suggest to take this in to consideration while implementing new functionality.