[PATCH] NSS: Fix service enumeration - sssd-devel - Fedora mailing-lists

List overview All Threads
Download

[PATCH] NSS: Fix service enumeration

[PATCH] MAN: Document that krb5...

[PATCH] ad_subdom_store: check ID...

Jakub Hrozek

29 Oct 2013 29 Oct '13

4:05 a.m.

The code wrote into the middle of the packet to a space that was already reserved and allocated but then still advanced the pointer to the buffer.

https://fedorahosted.org/sssd/ticket/2124

Attachments:

0001-NSS-Fix-service-enumeration.patch (text/plain — 1.0 KB)

Reply

Show replies by date

Lukas Slebodnik

29 Oct 29 Oct

9:24 a.m.

On (29/10/13 10:05), Jakub Hrozek wrote:

The code wrote into the middle of the packet to a space that was already reserved and allocated but then still advanced the pointer to the buffer.

https://fedorahosted.org/sssd/ticket/2124

From d533bb267ffb1447503f6af671a6d2d2db9ddd10 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 29 Oct 2013 07:10:30 +0100 Subject: [PATCH] NSS: Fix service enumeration

The code wrote into the middle of the packet to a space that was already reserved and allocated but then still advanced the pointer to the buffer.

https://fedorahosted.org/sssd/ticket/2124

src/responder/nss/nsssrv_services.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/responder/nss/nsssrv_services.c b/src/responder/nss/nsssrv_services.c index 79caa7d08cdfff25112eedab98a0419f1b1d154e..ff588592c814dc43e30f9e57ddab230efff15df2 100644 --- a/src/responder/nss/nsssrv_services.c +++ b/src/responder/nss/nsssrv_services.c @@ -752,7 +752,7 @@ fill_service(struct sss_packet *packet, written_aliases++; talloc_zfree(tmpstr); }
   SAFEALIGN_SET_UINT32(&body[aptr], written_aliases, &rsize);
   SAFEALIGN_SET_UINT32(&body[aptr], written_aliases, NULL);

  num++;
}
-- 1.8.3.1

Nice catch. We have already reserved place for variable written_aliases.

Here is code: ------------- ret = sss_packet_grow(packet, 2 * sizeof(uint16_t) + sizeof(uint32_t) // reserve place for variable written_aliases + cased_name.len + cased_proto.len); sss_packet_get_body(packet, &body, &blen);

/* Store the port number */ SAFEALIGN_SET_UINT32(&body[rzero + rsize], (uint32_t)htons(port), &rsize);

... snip ...

/* We'll store the alias count here */ aptr = rzero+rsize; <<< remember offset for variable written_aliases rsize += sizeof(uint32_t); <<< increment rsize

So it does not make sense to increment rsize twice and it was a reason why we could access behind allocated array.

Please add comment before line, where variable is written to packet, because it needn't be clear why rsize is not incremented.

I know that this patch will be backported to older branches. In this case, do we want to use new macro SAFEALIGN_SETMEM_UINT32?

LS

Reply

Jakub Hrozek

10:44 a.m.

On Tue, Oct 29, 2013 at 03:24:18PM +0100, Lukas Slebodnik wrote:

On (29/10/13 10:05), Jakub Hrozek wrote:

...
The code wrote into the middle of the packet to a space that was already reserved and allocated but then still advanced the pointer to the buffer.

https://fedorahosted.org/sssd/ticket/2124

...
From d533bb267ffb1447503f6af671a6d2d2db9ddd10 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 29 Oct 2013 07:10:30 +0100 Subject: [PATCH] NSS: Fix service enumeration

The code wrote into the middle of the packet to a space that was already reserved and allocated but then still advanced the pointer to the buffer.

https://fedorahosted.org/sssd/ticket/2124

src/responder/nss/nsssrv_services.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/responder/nss/nsssrv_services.c b/src/responder/nss/nsssrv_services.c index 79caa7d08cdfff25112eedab98a0419f1b1d154e..ff588592c814dc43e30f9e57ddab230efff15df2 100644 --- a/src/responder/nss/nsssrv_services.c +++ b/src/responder/nss/nsssrv_services.c @@ -752,7 +752,7 @@ fill_service(struct sss_packet *packet, written_aliases++; talloc_zfree(tmpstr); }
   SAFEALIGN_SET_UINT32(&body[aptr], written_aliases, &rsize);
   SAFEALIGN_SET_UINT32(&body[aptr], written_aliases, NULL);

  num++;
}
-- 1.8.3.1
Nice catch. We have already reserved place for variable written_aliases.

Here is code:

ret = sss_packet_grow(packet, 2 * sizeof(uint16_t) + sizeof(uint32_t) // reserve place for variable written_aliases + cased_name.len + cased_proto.len); sss_packet_get_body(packet, &body, &blen);

/* Store the port number */ SAFEALIGN_SET_UINT32(&body[rzero + rsize], (uint32_t)htons(port), &rsize);

... snip ...

/* We'll store the alias count here */ aptr = rzero+rsize; <<< remember offset for variable written_aliases rsize += sizeof(uint32_t); <<< increment rsize

So it does not make sense to increment rsize twice and it was a reason why we could access behind allocated array.

Please add comment before line, where variable is written to packet, because it needn't be clear why rsize is not incremented.

I know that this patch will be backported to older branches. In this case, do we want to use new macro SAFEALIGN_SETMEM_UINT32?

We can use it in master, but I would strongly prefer a separate patch so that it's easily visible what changed.

I added the comment.

Reply

Lukas Slebodnik

11:14 a.m.

On (29/10/13 16:44), Jakub Hrozek wrote:

On Tue, Oct 29, 2013 at 03:24:18PM +0100, Lukas Slebodnik wrote:

...
On (29/10/13 10:05), Jakub Hrozek wrote:

...
The code wrote into the middle of the packet to a space that was already reserved and allocated but then still advanced the pointer to the buffer.

https://fedorahosted.org/sssd/ticket/2124

...
From d533bb267ffb1447503f6af671a6d2d2db9ddd10 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 29 Oct 2013 07:10:30 +0100 Subject: [PATCH] NSS: Fix service enumeration

The code wrote into the middle of the packet to a space that was already reserved and allocated but then still advanced the pointer to the buffer.

https://fedorahosted.org/sssd/ticket/2124

src/responder/nss/nsssrv_services.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/responder/nss/nsssrv_services.c b/src/responder/nss/nsssrv_services.c index 79caa7d08cdfff25112eedab98a0419f1b1d154e..ff588592c814dc43e30f9e57ddab230efff15df2 100644 --- a/src/responder/nss/nsssrv_services.c +++ b/src/responder/nss/nsssrv_services.c @@ -752,7 +752,7 @@ fill_service(struct sss_packet *packet, written_aliases++; talloc_zfree(tmpstr); }
   SAFEALIGN_SET_UINT32(&body[aptr], written_aliases, &rsize);
   SAFEALIGN_SET_UINT32(&body[aptr], written_aliases, NULL);

  num++;
}
-- 1.8.3.1
Nice catch. We have already reserved place for variable written_aliases.

Here is code:

ret = sss_packet_grow(packet, 2 * sizeof(uint16_t) + sizeof(uint32_t) // reserve place for variable written_aliases + cased_name.len + cased_proto.len); sss_packet_get_body(packet, &body, &blen);

/* Store the port number */ SAFEALIGN_SET_UINT32(&body[rzero + rsize], (uint32_t)htons(port), &rsize);

... snip ...

/* We'll store the alias count here */ aptr = rzero+rsize; <<< remember offset for variable written_aliases rsize += sizeof(uint32_t); <<< increment rsize

So it does not make sense to increment rsize twice and it was a reason why we could access behind allocated array.

Please add comment before line, where variable is written to packet, because it needn't be clear why rsize is not incremented.

I know that this patch will be backported to older branches. In this case, do we want to use new macro SAFEALIGN_SETMEM_UINT32?
We can use it in master, but I would strongly prefer a separate patch so that it's easily visible what changed.

I added the comment.

From ab86dc24e9d5b5594e790cf4def23121b27d7f0e Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 29 Oct 2013 07:10:30 +0100 Subject: [PATCH 1/2] NSS: Fix service enumeration

I tested patch with master an sssd-1-9 branch.

ACK

From e137eaf703c1e5ca89d6523d3aa51e5e7a3448a1 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 29 Oct 2013 16:41:13 +0100 Subject: [PATCH 2/2] NSS: Use new safealign macros in NSS responder

ACK

LS

Reply

Jakub Hrozek

11:59 a.m.

On Tue, Oct 29, 2013 at 05:14:17PM +0100, Lukas Slebodnik wrote:

On (29/10/13 16:44), Jakub Hrozek wrote:

...
On Tue, Oct 29, 2013 at 03:24:18PM +0100, Lukas Slebodnik wrote:

...
On (29/10/13 10:05), Jakub Hrozek wrote:

...
The code wrote into the middle of the packet to a space that was already reserved and allocated but then still advanced the pointer to the buffer.

https://fedorahosted.org/sssd/ticket/2124

...
From d533bb267ffb1447503f6af671a6d2d2db9ddd10 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 29 Oct 2013 07:10:30 +0100 Subject: [PATCH] NSS: Fix service enumeration

The code wrote into the middle of the packet to a space that was already reserved and allocated but then still advanced the pointer to the buffer.

https://fedorahosted.org/sssd/ticket/2124

src/responder/nss/nsssrv_services.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/responder/nss/nsssrv_services.c b/src/responder/nss/nsssrv_services.c index 79caa7d08cdfff25112eedab98a0419f1b1d154e..ff588592c814dc43e30f9e57ddab230efff15df2 100644 --- a/src/responder/nss/nsssrv_services.c +++ b/src/responder/nss/nsssrv_services.c @@ -752,7 +752,7 @@ fill_service(struct sss_packet *packet, written_aliases++; talloc_zfree(tmpstr); }
   SAFEALIGN_SET_UINT32(&body[aptr], written_aliases, &rsize);
   SAFEALIGN_SET_UINT32(&body[aptr], written_aliases, NULL);

  num++;
}
-- 1.8.3.1
Nice catch. We have already reserved place for variable written_aliases.

Here is code:

ret = sss_packet_grow(packet, 2 * sizeof(uint16_t) + sizeof(uint32_t) // reserve place for variable written_aliases + cased_name.len + cased_proto.len); sss_packet_get_body(packet, &body, &blen);

/* Store the port number */ SAFEALIGN_SET_UINT32(&body[rzero + rsize], (uint32_t)htons(port), &rsize);

... snip ...

/* We'll store the alias count here */ aptr = rzero+rsize; <<< remember offset for variable written_aliases rsize += sizeof(uint32_t); <<< increment rsize

So it does not make sense to increment rsize twice and it was a reason why we could access behind allocated array.

Please add comment before line, where variable is written to packet, because it needn't be clear why rsize is not incremented.

I know that this patch will be backported to older branches. In this case, do we want to use new macro SAFEALIGN_SETMEM_UINT32?
We can use it in master, but I would strongly prefer a separate patch so that it's easily visible what changed.

I added the comment.
...
From ab86dc24e9d5b5594e790cf4def23121b27d7f0e Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 29 Oct 2013 07:10:30 +0100 Subject: [PATCH 1/2] NSS: Fix service enumeration

I tested patch with master an sssd-1-9 branch.

ACK

...
From e137eaf703c1e5ca89d6523d3aa51e5e7a3448a1 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 29 Oct 2013 16:41:13 +0100 Subject: [PATCH 2/2] NSS: Use new safealign macros in NSS responder

ACK

LS

Thank you for the review, both patches were pushed to master and patch #1 was pushed to sssd-1-11

Reply

4647

Age (days ago)

4647

Last active (days ago)

sssd-devel@lists.fedorahosted.org

4 comments

2 participants

tags (0)

participants (2)

Jakub Hrozek
Lukas Slebodnik