On (15/11/13 18:29), Michal Židek wrote:
On 11/14/2013 01:14 PM, Lukas Slebodnik wrote:>>From
fbc7adbf1331fdb931b43f2b6ffda43565ce442e Mon Sep 17 00:00:00 2001
>> >From: Michal Zidek<mzidek(a)redhat.com>
>> >Date: Wed, 4 Sep 2013 16:17:57 +0200
>> >Subject: [PATCH 5/7] responder: Use SAFEALIGN macro when checking
pam data
>> >validity.
>> >
>> >resolves:
>> >https://fedorahosted.org/sssd/ticket/1359
>> >---
>> >src/responder/pam/pamsrv_cmd.c | 18 +++++++++++++-----
>> >1 file changed, 13 insertions(+), 5 deletions(-)
>> >
>> >diff --git a/src/responder/pam/pamsrv_cmd.c
b/src/responder/pam/pamsrv_cmd.c
>> >index 32efc4b..6418047 100644
>> >--- a/src/responder/pam/pamsrv_cmd.c
>> >+++ b/src/responder/pam/pamsrv_cmd.c
>> >@@ -144,12 +144,20 @@ static int pam_parse_in_data_v2(struct
sss_domain_info *domains,
>> > uint32_t size;
>> > char *pam_user;
>> > int ret;
>> >- uint32_t terminator = SSS_END_OF_PAM_REQUEST;
>> >+ uint32_t start;
>> >+ uint32_t terminator;
>> >
>> >- if (blen < 4*sizeof(uint32_t)+2 ||
>> >- ((uint32_t *)body)[0] != SSS_START_OF_PAM_REQUEST ||
>> >- memcmp(&body[blen - sizeof(uint32_t)], &terminator,
sizeof(uint32_t)) != 0) {
> Function memcmp is also used in the function pam_forwarder_parse_data,
> although there is no alignment warning we should be consistent at
least in the
> same file.
Yes. I modified it to use safealign copy as well in the new patch.
Thank you.
Works fine, warnings are gone.
ACK
Michal
PS:
Btw. why is the condition to detect unterminated data like this
if (blen >= sizeof(uint32_t)
&& lastfourbytes != END_OF_PAM_REQUEST) {
return error;
}
Shouldn't it be like this?
if (blen < sizeof(uint32_t)
|| lastfourbytes != END_OF_PAM_REQUEST) {
return error;
}
Is it possible that pam request shorter than terminator is valid?
If you think it is
a wrong you should send another patch.
I prefer simple commit which change only one thing.
LS