x509: Fix possible use-after-free when OOM #21040

neverpanic · 2023-05-24T10:30:58Z

ossl_policy_level_add_node() first adds the new node to the level->nodes stack, and then attempts to add extra data if extra_data is true. If memory allocation or adding the extra data to tree->extra_data fails, the allocated node (that has already been added to the level->nodes stack) is freed using ossl_policy_node_free(), which leads to a potential use after free.

Additionally, the tree's node count and the parent's child count would not be updated, despite the new node being added.

Checklist

~~documentation is added or updated~~
~~tests are added or updated~~

crypto/x509/pcy_node.c

crypto/x509/pcy_tree.c

crypto/x509/pcy_node.c

crypto/x509/pcy_tree.c

crypto/x509/pcy_node.c

ossl_policy_level_add_node() first adds the new node to the level->nodes stack, and then attempts to add extra data if extra_data is true. If memory allocation or adding the extra data to tree->extra_data fails, the allocated node (that has already been added to the level->nodes stack) is freed using ossl_policy_node_free(), which leads to a potential use after free. Additionally, the tree's node count and the parent's child count would not be updated, despite the new node being added. Fix this by either performing the function's purpose completely, or not at all by reverting the changes on error. Signed-off-by: Clemens Lang <cllang@redhat.com>

The invocation of ossl_policy_level_add_node in tree_calculate_user_set did not have any error handling. Add it to prevent a memory leak for the allocated extra policy data. Also add error handling to sk_X509_POLICY_NODE_push to ensure that if a new node was allocated, but could not be added to the stack, it is freed correctly. Fix error handling if tree->user_policies cannot be allocated by returning 0, indicating failure, rather than 1. Signed-off-by: Clemens Lang <cllang@redhat.com>

beldmit

LGTM

t8m · 2023-05-26T10:04:01Z

Backport to 1.1.1 in #21066

openssl-machine · 2023-05-27T10:00:15Z

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

ossl_policy_level_add_node() first adds the new node to the level->nodes stack, and then attempts to add extra data if extra_data is true. If memory allocation or adding the extra data to tree->extra_data fails, the allocated node (that has already been added to the level->nodes stack) is freed using ossl_policy_node_free(), which leads to a potential use after free. Additionally, the tree's node count and the parent's child count would not be updated, despite the new node being added. Fix this by either performing the function's purpose completely, or not at all by reverting the changes on error. Signed-off-by: Clemens Lang <cllang@redhat.com> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21040)

The invocation of ossl_policy_level_add_node in tree_calculate_user_set did not have any error handling. Add it to prevent a memory leak for the allocated extra policy data. Also add error handling to sk_X509_POLICY_NODE_push to ensure that if a new node was allocated, but could not be added to the stack, it is freed correctly. Fix error handling if tree->user_policies cannot be allocated by returning 0, indicating failure, rather than 1. Signed-off-by: Clemens Lang <cllang@redhat.com> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21040)

t8m · 2023-05-29T12:56:42Z

Merged to master, 3.1, and 3.0 branches. Thank you for your contribution.

ossl_policy_level_add_node() first adds the new node to the level->nodes stack, and then attempts to add extra data if extra_data is true. If memory allocation or adding the extra data to tree->extra_data fails, the allocated node (that has already been added to the level->nodes stack) is freed using ossl_policy_node_free(), which leads to a potential use after free. Additionally, the tree's node count and the parent's child count would not be updated, despite the new node being added. Fix this by either performing the function's purpose completely, or not at all by reverting the changes on error. Signed-off-by: Clemens Lang <cllang@redhat.com> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21040) (cherry picked from commit de53817)

The invocation of ossl_policy_level_add_node in tree_calculate_user_set did not have any error handling. Add it to prevent a memory leak for the allocated extra policy data. Also add error handling to sk_X509_POLICY_NODE_push to ensure that if a new node was allocated, but could not be added to the stack, it is freed correctly. Fix error handling if tree->user_policies cannot be allocated by returning 0, indicating failure, rather than 1. Signed-off-by: Clemens Lang <cllang@redhat.com> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21040) (cherry picked from commit 95a8aa6)

ossl_policy_level_add_node() first adds the new node to the level->nodes stack, and then attempts to add extra data if extra_data is true. If memory allocation or adding the extra data to tree->extra_data fails, the allocated node (that has already been added to the level->nodes stack) is freed using ossl_policy_node_free(), which leads to a potential use after free. Additionally, the tree's node count and the parent's child count would not be updated, despite the new node being added. Fix this by either performing the function's purpose completely, or not at all by reverting the changes on error. Signed-off-by: Clemens Lang <cllang@redhat.com> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21040) (cherry picked from commit de53817)

The invocation of ossl_policy_level_add_node in tree_calculate_user_set did not have any error handling. Add it to prevent a memory leak for the allocated extra policy data. Also add error handling to sk_X509_POLICY_NODE_push to ensure that if a new node was allocated, but could not be added to the stack, it is freed correctly. Fix error handling if tree->user_policies cannot be allocated by returning 0, indicating failure, rather than 1. Signed-off-by: Clemens Lang <cllang@redhat.com> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #21040) (cherry picked from commit 95a8aa6)

ossl_policy_level_add_node() first adds the new node to the level->nodes stack, and then attempts to add extra data if extra_data is true. If memory allocation or adding the extra data to tree->extra_data fails, the allocated node (that has already been added to the level->nodes stack) is freed using ossl_policy_node_free(), which leads to a potential use after free. Additionally, the tree's node count and the parent's child count would not be updated, despite the new node being added. Fix this by either performing the function's purpose completely, or not at all by reverting the changes on error. Signed-off-by: Clemens Lang <cllang@redhat.com> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl/openssl#21040)

The invocation of ossl_policy_level_add_node in tree_calculate_user_set did not have any error handling. Add it to prevent a memory leak for the allocated extra policy data. Also add error handling to sk_X509_POLICY_NODE_push to ensure that if a new node was allocated, but could not be added to the stack, it is freed correctly. Fix error handling if tree->user_policies cannot be allocated by returning 0, indicating failure, rather than 1. Signed-off-by: Clemens Lang <cllang@redhat.com> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl/openssl#21040)

bernd-edlinger requested changes May 24, 2023

View reviewed changes

crypto/x509/pcy_node.c Show resolved Hide resolved

mattcaswell reviewed May 24, 2023

View reviewed changes