Re: ber.c fix for length calculations

2018-06-25 Thread Claudio Jeker
On Sun, Jun 24, 2018 at 05:58:09PM -0400, Rob Pierce wrote:
> It looks like a BER problem found while testing the new ldap client (with an
> empty password) was already addressed in snmpd back in 2010 by martinh.
> 
> In LDAP under a CONTEXT class, 0 corresponds to LDAP_AUTH_SIMPLE. This is
> currently interpreted as an EOC (end-of-content) and causes a
> miscalculation of the length which results in unexpected behaviour.
> 
> Reyk pointed me in the right direction, and after much messing around I came
> up with a solution which was close to the following snmpd commit.
> 
> From the src/usr.sbin/snmpd/ber.c commit log:
> 
> 
> revision 1.23
> date: 2010/09/20 08:30:13;  author: martinh;  state: Exp;  lines: +3 -2;
> Allow output of null values with a context class. This is used in SNMPv2 to
> return an error exception value for a varbind result ("noSuchObject[0]
> IMPLICIT NULL" in rfc1905).
> 
> 
> However, BER_TYPE_EOC only applies to the UNIVERSAL class, so I think an
> explicit check against BER_CLASS_UNIVERSAL is a better solution and works
> across both ldap and snmp.
> 
> The following diff applies this change to ldap, ldapd, and ypldap, and brings
> snmpd in line with this approach.
> 
> Thoughts?
> 
> Ok?

I think the analysis is correct (my BER knowledge is rusty) and the fix
looks good. OK claudio@
 
> Index: usr.bin/ldap/ber.c
> ===
> RCS file: /cvs/src/usr.bin/ldap/ber.c,v
> retrieving revision 1.1.1.1
> diff -u -p -r1.1.1.1 ber.c
> --- usr.bin/ldap/ber.c13 Jun 2018 15:45:57 -  1.1.1.1
> +++ usr.bin/ldap/ber.c24 Jun 2018 21:37:39 -
> @@ -861,7 +861,8 @@ ber_calc_len(struct ber_element *root)
>   size += ber_calc_len(root->be_next);
>  
>   /* This is an empty element, do not use a minimal size */
> - if (root->be_type == BER_TYPE_EOC && root->be_len == 0)
> + if (root->be_class == BER_CLASS_UNIVERSAL &&
> + root->be_type == BER_TYPE_EOC && root->be_len == 0)
>   return (0);
>  
>   return (root->be_len + size);
> 
> Index: usr.sbin/ldapd/ber.c
> ===
> RCS file: /cvs/src/usr.sbin/ldapd/ber.c,v
> retrieving revision 1.13
> diff -u -p -r1.13 ber.c
> --- usr.sbin/ldapd/ber.c  8 Feb 2018 18:02:06 -   1.13
> +++ usr.sbin/ldapd/ber.c  24 Jun 2018 21:37:39 -
> @@ -874,7 +874,8 @@ ber_calc_len(struct ber_element *root)
>   size += ber_calc_len(root->be_next);
>  
>   /* This is an empty element, do not use a minimal size */
> - if (root->be_type == BER_TYPE_EOC && root->be_len == 0)
> + if (root->be_class == BER_CLASS_UNIVERSAL &&
> + root->be_type == BER_TYPE_EOC && root->be_len == 0)
>   return (0);
>  
>   return (root->be_len + size);
> 
> Index: usr.sbin/ypldap/ber.c
> ===
> RCS file: /cvs/src/usr.sbin/ypldap/ber.c,v
> retrieving revision 1.13
> diff -u -p -r1.13 ber.c
> --- usr.sbin/ypldap/ber.c 8 Feb 2018 18:02:06 -   1.13
> +++ usr.sbin/ypldap/ber.c 24 Jun 2018 21:37:39 -
> @@ -861,7 +861,8 @@ ber_calc_len(struct ber_element *root)
>   size += ber_calc_len(root->be_next);
>  
>   /* This is an empty element, do not use a minimal size */
> - if (root->be_type == BER_TYPE_EOC && root->be_len == 0)
> + if (root->be_class == BER_CLASS_UNIVERSAL &&
> + root->be_type == BER_TYPE_EOC && root->be_len == 0)
>   return (0);
>  
>   return (root->be_len + size);
> 
> Index: usr.sbin/snmpd/ber.c
> ===
> RCS file: /cvs/src/usr.sbin/snmpd/ber.c,v
> retrieving revision 1.32
> diff -u -p -r1.32 ber.c
> --- usr.sbin/snmpd/ber.c  8 Feb 2018 18:02:06 -   1.32
> +++ usr.sbin/snmpd/ber.c  24 Jun 2018 21:37:39 -
> @@ -880,7 +880,7 @@ ber_calc_len(struct ber_element *root)
>   size += ber_calc_len(root->be_next);
>  
>   /* This is an empty element, do not use a minimal size */
> - if (root->be_class != BER_CLASS_CONTEXT &&
> + if (root->be_class == BER_CLASS_UNIVERSAL &&
>   root->be_type == BER_TYPE_EOC && root->be_len == 0)
>   return (0);
>  
> 

-- 
:wq Claudio



ber.c fix for length calculations

2018-06-24 Thread Rob Pierce
It looks like a BER problem found while testing the new ldap client (with an
empty password) was already addressed in snmpd back in 2010 by martinh.

In LDAP under a CONTEXT class, 0 corresponds to LDAP_AUTH_SIMPLE. This is
currently interpreted as an EOC (end-of-content) and causes a
miscalculation of the length which results in unexpected behaviour.

Reyk pointed me in the right direction, and after much messing around I came
up with a solution which was close to the following snmpd commit.

>From the src/usr.sbin/snmpd/ber.c commit log:


revision 1.23
date: 2010/09/20 08:30:13;  author: martinh;  state: Exp;  lines: +3 -2;
Allow output of null values with a context class. This is used in SNMPv2 to
return an error exception value for a varbind result ("noSuchObject[0]
IMPLICIT NULL" in rfc1905).


However, BER_TYPE_EOC only applies to the UNIVERSAL class, so I think an
explicit check against BER_CLASS_UNIVERSAL is a better solution and works
across both ldap and snmp.

The following diff applies this change to ldap, ldapd, and ypldap, and brings
snmpd in line with this approach.

Thoughts?

Ok?

Index: usr.bin/ldap/ber.c
===
RCS file: /cvs/src/usr.bin/ldap/ber.c,v
retrieving revision 1.1.1.1
diff -u -p -r1.1.1.1 ber.c
--- usr.bin/ldap/ber.c  13 Jun 2018 15:45:57 -  1.1.1.1
+++ usr.bin/ldap/ber.c  24 Jun 2018 21:37:39 -
@@ -861,7 +861,8 @@ ber_calc_len(struct ber_element *root)
size += ber_calc_len(root->be_next);
 
/* This is an empty element, do not use a minimal size */
-   if (root->be_type == BER_TYPE_EOC && root->be_len == 0)
+   if (root->be_class == BER_CLASS_UNIVERSAL &&
+   root->be_type == BER_TYPE_EOC && root->be_len == 0)
return (0);
 
return (root->be_len + size);

Index: usr.sbin/ldapd/ber.c
===
RCS file: /cvs/src/usr.sbin/ldapd/ber.c,v
retrieving revision 1.13
diff -u -p -r1.13 ber.c
--- usr.sbin/ldapd/ber.c8 Feb 2018 18:02:06 -   1.13
+++ usr.sbin/ldapd/ber.c24 Jun 2018 21:37:39 -
@@ -874,7 +874,8 @@ ber_calc_len(struct ber_element *root)
size += ber_calc_len(root->be_next);
 
/* This is an empty element, do not use a minimal size */
-   if (root->be_type == BER_TYPE_EOC && root->be_len == 0)
+   if (root->be_class == BER_CLASS_UNIVERSAL &&
+   root->be_type == BER_TYPE_EOC && root->be_len == 0)
return (0);
 
return (root->be_len + size);

Index: usr.sbin/ypldap/ber.c
===
RCS file: /cvs/src/usr.sbin/ypldap/ber.c,v
retrieving revision 1.13
diff -u -p -r1.13 ber.c
--- usr.sbin/ypldap/ber.c   8 Feb 2018 18:02:06 -   1.13
+++ usr.sbin/ypldap/ber.c   24 Jun 2018 21:37:39 -
@@ -861,7 +861,8 @@ ber_calc_len(struct ber_element *root)
size += ber_calc_len(root->be_next);
 
/* This is an empty element, do not use a minimal size */
-   if (root->be_type == BER_TYPE_EOC && root->be_len == 0)
+   if (root->be_class == BER_CLASS_UNIVERSAL &&
+   root->be_type == BER_TYPE_EOC && root->be_len == 0)
return (0);
 
return (root->be_len + size);

Index: usr.sbin/snmpd/ber.c
===
RCS file: /cvs/src/usr.sbin/snmpd/ber.c,v
retrieving revision 1.32
diff -u -p -r1.32 ber.c
--- usr.sbin/snmpd/ber.c8 Feb 2018 18:02:06 -   1.32
+++ usr.sbin/snmpd/ber.c24 Jun 2018 21:37:39 -
@@ -880,7 +880,7 @@ ber_calc_len(struct ber_element *root)
size += ber_calc_len(root->be_next);
 
/* This is an empty element, do not use a minimal size */
-   if (root->be_class != BER_CLASS_CONTEXT &&
+   if (root->be_class == BER_CLASS_UNIVERSAL &&
root->be_type == BER_TYPE_EOC && root->be_len == 0)
return (0);