From 279bdb5148eb0b67ddab40c4dd9d08e9e1672f13 Mon Sep 17 00:00:00 2001

From: William Brown <william@blackhats.net.au>

Date: Fri, 26 Jun 2020 10:27:56 +1000

Subject: [PATCH 07/12] Ticket 51175 - resolve plugin name leaking

Bug Description: Previously pblock.c assumed that all plugin

names were static c strings. Rust can't create static C

strings, so these were intentionally leaked.

Fix Description: Rather than leak these, we do a dup/free

through the slapiplugin struct instead, meaning we can use

ephemeral, and properly managed strings in rust. This does not

affect any other existing code which will still handle the

static strings correctly.

https://pagure.io/389-ds-base/issue/51175

Author: William Brown <william@blackhats.net.au>

Review by: mreynolds, tbordaz (Thanks!)

---

 Makefile.am                             |  1 +

 configure.ac                            |  2 +-

 ldap/servers/slapd/pagedresults.c       |  6 +--

 ldap/servers/slapd/pblock.c             |  9 ++--

 ldap/servers/slapd/plugin.c             |  7 +++

 ldap/servers/slapd/pw_verify.c          |  1 +

 ldap/servers/slapd/tools/pwenc.c        |  2 +-

 src/slapi_r_plugin/README.md            |  6 +--

 src/slapi_r_plugin/src/charray.rs       | 32 ++++++++++++++

 src/slapi_r_plugin/src/lib.rs           |  8 ++--

 src/slapi_r_plugin/src/macros.rs        | 17 +++++---

 src/slapi_r_plugin/src/syntax_plugin.rs | 57 +++++++------------------

 12 files changed, 85 insertions(+), 63 deletions(-)

 create mode 100644 src/slapi_r_plugin/src/charray.rs

diff --git a/Makefile.am b/Makefile.am

index 627953850..36434cf17 100644

--- a/Makefile.am

+++ b/Makefile.am

@@ -1312,6 +1312,7 @@ rust-nsslapd-private.h: @abs_top_builddir@/rs/@rust_target_dir@/librnsslapd.a

 libslapi_r_plugin_SOURCES = \

 	src/slapi_r_plugin/src/backend.rs \

 	src/slapi_r_plugin/src/ber.rs \

+	src/slapi_r_plugin/src/charray.rs \

 	src/slapi_r_plugin/src/constants.rs \

 	src/slapi_r_plugin/src/dn.rs \

 	src/slapi_r_plugin/src/entry.rs \

diff --git a/configure.ac b/configure.ac

index b3cf77d08..61bf35e4a 100644

--- a/configure.ac

+++ b/configure.ac

@@ -122,7 +122,7 @@ if test "$enable_debug" = yes ; then

   debug_defs="-DDEBUG -DMCC_DEBUG"

   debug_cflags="-g3 -O0 -rdynamic"

   debug_cxxflags="-g3 -O0 -rdynamic"

-  debug_rust_defs="-C debuginfo=2"

+  debug_rust_defs="-C debuginfo=2 -Z macro-backtrace"

   cargo_defs=""

   rust_target_dir="debug"

 else

diff --git a/ldap/servers/slapd/pagedresults.c b/ldap/servers/slapd/pagedresults.c

index d8b8798b6..e3444e944 100644

--- a/ldap/servers/slapd/pagedresults.c

+++ b/ldap/servers/slapd/pagedresults.c

@@ -738,10 +738,10 @@ pagedresults_cleanup(Connection *conn, int needlock)

     int i;

     PagedResults *prp = NULL;

-    slapi_log_err(SLAPI_LOG_TRACE, "pagedresults_cleanup", "=>\n");

+    /* slapi_log_err(SLAPI_LOG_TRACE, "pagedresults_cleanup", "=>\n"); */

     if (NULL == conn) {

-        slapi_log_err(SLAPI_LOG_TRACE, "pagedresults_cleanup", "<= Connection is NULL\n");

+        /* slapi_log_err(SLAPI_LOG_TRACE, "pagedresults_cleanup", "<= Connection is NULL\n"); */

         return 0;

     }

@@ -767,7 +767,7 @@ pagedresults_cleanup(Connection *conn, int needlock)

     if (needlock) {

         pthread_mutex_unlock(&(conn->c_mutex));

     }

-    slapi_log_err(SLAPI_LOG_TRACE, "pagedresults_cleanup", "<= %d\n", rc);

+    /* slapi_log_err(SLAPI_LOG_TRACE, "pagedresults_cleanup", "<= %d\n", rc); */

     return rc;

 }

diff --git a/ldap/servers/slapd/pblock.c b/ldap/servers/slapd/pblock.c

index 1ad9d0399..f7d1f8885 100644

--- a/ldap/servers/slapd/pblock.c

+++ b/ldap/servers/slapd/pblock.c

@@ -3351,13 +3351,15 @@ slapi_pblock_set(Slapi_PBlock *pblock, int arg, void *value)

         if (pblock->pb_plugin->plg_type != SLAPI_PLUGIN_SYNTAX) {

             return (-1);

         }

-        pblock->pb_plugin->plg_syntax_names = (char **)value;

+        PR_ASSERT(pblock->pb_plugin->plg_syntax_names == NULL);

+        pblock->pb_plugin->plg_syntax_names = slapi_ch_array_dup((char **)value);

         break;

     case SLAPI_PLUGIN_SYNTAX_OID:

         if (pblock->pb_plugin->plg_type != SLAPI_PLUGIN_SYNTAX) {

             return (-1);

         }

-        pblock->pb_plugin->plg_syntax_oid = (char *)value;

+        PR_ASSERT(pblock->pb_plugin->plg_syntax_oid == NULL);

+        pblock->pb_plugin->plg_syntax_oid = slapi_ch_strdup((char *)value);

         break;

     case SLAPI_PLUGIN_SYNTAX_FLAGS:

         if (pblock->pb_plugin->plg_type != SLAPI_PLUGIN_SYNTAX) {

@@ -3806,7 +3808,8 @@ slapi_pblock_set(Slapi_PBlock *pblock, int arg, void *value)

         if (pblock->pb_plugin->plg_type != SLAPI_PLUGIN_MATCHINGRULE) {

             return (-1);

         }

-        pblock->pb_plugin->plg_mr_names = (char **)value;

+        PR_ASSERT(pblock->pb_plugin->plg_mr_names == NULL);

+        pblock->pb_plugin->plg_mr_names = slapi_ch_array_dup((char **)value);

         break;

     case SLAPI_PLUGIN_MR_COMPARE:

         if (pblock->pb_plugin->plg_type != SLAPI_PLUGIN_MATCHINGRULE) {

diff --git a/ldap/servers/slapd/plugin.c b/ldap/servers/slapd/plugin.c

index 282b98738..e6b48de60 100644

--- a/ldap/servers/slapd/plugin.c

+++ b/ldap/servers/slapd/plugin.c

@@ -2694,6 +2694,13 @@ plugin_free(struct slapdplugin *plugin)

     if (plugin->plg_type == SLAPI_PLUGIN_PWD_STORAGE_SCHEME || plugin->plg_type == SLAPI_PLUGIN_REVER_PWD_STORAGE_SCHEME) {

         slapi_ch_free_string(&plugin->plg_pwdstorageschemename);

     }

+    if (plugin->plg_type == SLAPI_PLUGIN_SYNTAX) {

+        slapi_ch_free_string(&plugin->plg_syntax_oid);

+        slapi_ch_array_free(plugin->plg_syntax_names);

+    }

+    if (plugin->plg_type == SLAPI_PLUGIN_MATCHINGRULE) {

+        slapi_ch_array_free(plugin->plg_mr_names);

+    }

     release_componentid(plugin->plg_identity);

     slapi_counter_destroy(&plugin->plg_op_counter);

     if (!plugin->plg_group) {

diff --git a/ldap/servers/slapd/pw_verify.c b/ldap/servers/slapd/pw_verify.c

index 4f0944b73..4ff1fa2fd 100644

--- a/ldap/servers/slapd/pw_verify.c

+++ b/ldap/servers/slapd/pw_verify.c

@@ -111,6 +111,7 @@ pw_verify_token_dn(Slapi_PBlock *pb) {

     if (fernet_verify_token(dn, cred->bv_val, key, tok_ttl) != 0) {

         rc = SLAPI_BIND_SUCCESS;

     }

+    slapi_ch_free_string(&key);

 #endif

     return rc;

 }

diff --git a/ldap/servers/slapd/tools/pwenc.c b/ldap/servers/slapd/tools/pwenc.c

index 1629c06cd..d89225e34 100644

--- a/ldap/servers/slapd/tools/pwenc.c

+++ b/ldap/servers/slapd/tools/pwenc.c

@@ -34,7 +34,7 @@

 int ldap_syslog;

 int ldap_syslog_level;

-int slapd_ldap_debug = LDAP_DEBUG_ANY;

+/* int slapd_ldap_debug = LDAP_DEBUG_ANY; */

 int detached;

 FILE *error_logfp;

 FILE *access_logfp;

diff --git a/src/slapi_r_plugin/README.md b/src/slapi_r_plugin/README.md

index af9743ec9..1c9bcbf17 100644

--- a/src/slapi_r_plugin/README.md

+++ b/src/slapi_r_plugin/README.md

@@ -15,7 +15,7 @@ the [Rust Nomicon](https://doc.rust-lang.org/nomicon/index.html)

 > warning about danger.

 This document will not detail the specifics of unsafe or the invariants you must adhere to for rust

-to work with C.

+to work with C. Failure to uphold these invariants will lead to less than optimal consequences.

 If you still want to see more about the plugin bindings, go on ...

@@ -135,7 +135,7 @@ associated functions.

 Now, you may notice that not all members of the trait are implemented. This is due to a feature

 of rust known as default trait impls. This allows the trait origin (src/plugin.rs) to provide

 template versions of these functions. If you "overwrite" them, your implementation is used. Unlike

-OO, you may not inherit or call the default function. 

+OO, you may not inherit or call the default function.

 If a default is not provided you *must* implement that function to be considered valid. Today (20200422)

 this only applies to `start` and `close`.

@@ -183,7 +183,7 @@ It's important to understand how Rust manages memory both on the stack and the h

 As a result, this means that we must express in code, assertions about the proper ownership of memory

 and who is responsible for it (unlike C, where it can be hard to determine who or what is responsible

 for freeing some value.) Failure to handle this correctly, can and will lead to crashes, leaks or

-*hand waving* magical failures that are eXtReMeLy FuN to debug.

+*hand waving* magical failures that are `eXtReMeLy FuN` to debug.

 ### Reference Types

diff --git a/src/slapi_r_plugin/src/charray.rs b/src/slapi_r_plugin/src/charray.rs

new file mode 100644

index 000000000..d2e44693c

--- /dev/null

+++ b/src/slapi_r_plugin/src/charray.rs

@@ -0,0 +1,32 @@

+use std::ffi::CString;

+use std::iter::once;

+use std::os::raw::c_char;

+use std::ptr;

+

+pub struct Charray {

+    pin: Vec<CString>,

+    charray: Vec<*const c_char>,

+}

+

+impl Charray {

+    pub fn new(input: &[&str]) -> Result<Self, ()> {

+        let pin: Result<Vec<_>, ()> = input

+            .iter()

+            .map(|s| CString::new(*s).map_err(|_e| ()))

+            .collect();

+

+        let pin = pin?;

+

+        let charray: Vec<_> = pin

+            .iter()

+            .map(|s| s.as_ptr())

+            .chain(once(ptr::null()))

+            .collect();

+

+        Ok(Charray { pin, charray })

+    }

+

+    pub fn as_ptr(&self) -> *const *const c_char {

+        self.charray.as_ptr()

+    }

+}

diff --git a/src/slapi_r_plugin/src/lib.rs b/src/slapi_r_plugin/src/lib.rs

index 076907bae..be28cac95 100644

--- a/src/slapi_r_plugin/src/lib.rs

+++ b/src/slapi_r_plugin/src/lib.rs

@@ -1,9 +1,11 @@

-// extern crate lazy_static;

+#[macro_use]

+extern crate lazy_static;

 #[macro_use]

 pub mod macros;

 pub mod backend;

 pub mod ber;

+pub mod charray;

 mod constants;

 pub mod dn;

 pub mod entry;

@@ -20,6 +22,7 @@ pub mod value;

 pub mod prelude {

     pub use crate::backend::{BackendRef, BackendRefTxn};

     pub use crate::ber::BerValRef;

+    pub use crate::charray::Charray;

     pub use crate::constants::{FilterType, PluginFnType, PluginType, PluginVersion, LDAP_SUCCESS};

     pub use crate::dn::{Sdn, SdnRef};

     pub use crate::entry::EntryRef;

@@ -30,8 +33,7 @@ pub mod prelude {

     pub use crate::plugin::{register_plugin_ext, PluginIdRef, SlapiPlugin3};

     pub use crate::search::{Search, SearchScope};

     pub use crate::syntax_plugin::{

-        matchingrule_register, name_to_leaking_char, names_to_leaking_char_array, SlapiOrdMr,

-        SlapiSubMr, SlapiSyntaxPlugin1,

+        matchingrule_register, SlapiOrdMr, SlapiSubMr, SlapiSyntaxPlugin1,

     };

     pub use crate::task::{task_register_handler_fn, task_unregister_handler_fn, Task, TaskRef};

     pub use crate::value::{Value, ValueArray, ValueArrayRef, ValueRef};

diff --git a/src/slapi_r_plugin/src/macros.rs b/src/slapi_r_plugin/src/macros.rs

index bc8dfa60f..97fc5d7ef 100644

--- a/src/slapi_r_plugin/src/macros.rs

+++ b/src/slapi_r_plugin/src/macros.rs

@@ -249,6 +249,7 @@ macro_rules! slapi_r_syntax_plugin_hooks {

         paste::item! {

             use libc;

             use std::convert::TryFrom;

+            use std::ffi::CString;

             #[no_mangle]

             pub extern "C" fn [<$mod_ident _plugin_init>](raw_pb: *const libc::c_void) -> i32 {

@@ -261,15 +262,15 @@ macro_rules! slapi_r_syntax_plugin_hooks {

                 };

                 // Setup the names/oids that this plugin provides syntaxes for.

-

-                let name_ptr = unsafe { names_to_leaking_char_array(&$hooks_ident::attr_supported_names()) };

-                match pb.register_syntax_names(name_ptr) {

+                // DS will clone these, so they can be ephemeral to this function.

+                let name_vec = Charray::new($hooks_ident::attr_supported_names().as_slice()).expect("invalid supported names");

+                match pb.register_syntax_names(name_vec.as_ptr()) {

                     0 => {},

                     e => return e,

                 };

-                let name_ptr = unsafe { name_to_leaking_char($hooks_ident::attr_oid()) };

-                match pb.register_syntax_oid(name_ptr) {

+                let attr_oid = CString::new($hooks_ident::attr_oid()).expect("invalid attr oid");

+                match pb.register_syntax_oid(attr_oid.as_ptr()) {

                     0 => {},

                     e => return e,

                 };

@@ -430,7 +431,8 @@ macro_rules! slapi_r_syntax_plugin_hooks {

                     e => return e,

                 };

-                let name_ptr = unsafe { names_to_leaking_char_array(&$hooks_ident::eq_mr_supported_names()) };

+                let name_vec = Charray::new($hooks_ident::eq_mr_supported_names().as_slice()).expect("invalid mr supported names");

+                let name_ptr = name_vec.as_ptr();

                 // SLAPI_PLUGIN_MR_NAMES

                 match pb.register_mr_names(name_ptr) {

                     0 => {},

@@ -672,7 +674,8 @@ macro_rules! slapi_r_syntax_plugin_hooks {

                     e => return e,

                 };

-                let name_ptr = unsafe { names_to_leaking_char_array(&$hooks_ident::ord_mr_supported_names()) };

+                let name_vec = Charray::new($hooks_ident::ord_mr_supported_names().as_slice()).expect("invalid ord supported names");

+                let name_ptr = name_vec.as_ptr();

                 // SLAPI_PLUGIN_MR_NAMES

                 match pb.register_mr_names(name_ptr) {

                     0 => {},

diff --git a/src/slapi_r_plugin/src/syntax_plugin.rs b/src/slapi_r_plugin/src/syntax_plugin.rs

index e7d5c01bd..86f84bdd8 100644

--- a/src/slapi_r_plugin/src/syntax_plugin.rs

+++ b/src/slapi_r_plugin/src/syntax_plugin.rs

@@ -1,11 +1,11 @@

 use crate::ber::BerValRef;

 // use crate::constants::FilterType;

+use crate::charray::Charray;

 use crate::error::PluginError;

 use crate::pblock::PblockRef;

 use crate::value::{ValueArray, ValueArrayRef};

 use std::cmp::Ordering;

 use std::ffi::CString;

-use std::iter::once;

 use std::os::raw::c_char;

 use std::ptr;

@@ -26,37 +26,6 @@ struct slapi_matchingRuleEntry {

     mr_compat_syntax: *const *const c_char,

 }

-pub unsafe fn name_to_leaking_char(name: &str) -> *const c_char {

-    let n = CString::new(name)

-        .expect("An invalid string has been hardcoded!")

-        .into_boxed_c_str();

-    let n_ptr = n.as_ptr();

-    // Now we intentionally leak the name here, and the pointer will remain valid.

-    Box::leak(n);

-    n_ptr

-}

-

-pub unsafe fn names_to_leaking_char_array(names: &[&str]) -> *const *const c_char {

-    let n_arr: Vec<CString> = names

-        .iter()

-        .map(|s| CString::new(*s).expect("An invalid string has been hardcoded!"))

-        .collect();

-    let n_arr = n_arr.into_boxed_slice();

-    let n_ptr_arr: Vec<*const c_char> = n_arr

-        .iter()

-        .map(|v| v.as_ptr())

-        .chain(once(ptr::null()))

-        .collect();

-    let n_ptr_arr = n_ptr_arr.into_boxed_slice();

-

-    // Now we intentionally leak these names here,

-    let _r_n_arr = Box::leak(n_arr);

-    let r_n_ptr_arr = Box::leak(n_ptr_arr);

-

-    let name_ptr = r_n_ptr_arr as *const _ as *const *const c_char;

-    name_ptr

-}

-

 // oid - the oid of the matching rule

 // name - the name of the mr

 // desc - description

@@ -69,20 +38,24 @@ pub unsafe fn matchingrule_register(

     syntax: &str,

     compat_syntax: &[&str],

 ) -> i32 {

-    let oid_ptr = name_to_leaking_char(oid);

-    let name_ptr = name_to_leaking_char(name);

-    let desc_ptr = name_to_leaking_char(desc);

-    let syntax_ptr = name_to_leaking_char(syntax);

-    let compat_syntax_ptr = names_to_leaking_char_array(compat_syntax);

+    // Make everything CStrings that live long enough.

+

+    let oid_cs = CString::new(oid).expect("invalid oid");

+    let name_cs = CString::new(name).expect("invalid name");

+    let desc_cs = CString::new(desc).expect("invalid desc");

+    let syntax_cs = CString::new(syntax).expect("invalid syntax");

+

+    // We have to do this so the cstrings live long enough.

+    let compat_syntax_ca = Charray::new(compat_syntax).expect("invalid compat_syntax");

     let new_mr = slapi_matchingRuleEntry {

-        mr_oid: oid_ptr,

+        mr_oid: oid_cs.as_ptr(),

         _mr_oidalias: ptr::null(),

-        mr_name: name_ptr,

-        mr_desc: desc_ptr,

-        mr_syntax: syntax_ptr,

+        mr_name: name_cs.as_ptr(),

+        mr_desc: desc_cs.as_ptr(),

+        mr_syntax: syntax_cs.as_ptr(),

         _mr_obsolete: 0,

-        mr_compat_syntax: compat_syntax_ptr,

+        mr_compat_syntax: compat_syntax_ca.as_ptr(),

     };

     let new_mr_ptr = &new_mr as *const _;

-- 

2.26.3