Re: RADIUS authentication

(2010/01/24 23:29), Magnus Hagander wrote:
> 2010/1/20 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> (2010/01/20 0:19), Magnus Hagander wrote:
>>>> * I think this comment is right.
>>>> + for (i = 0; i< RADIUS_VECTOR_LENGTH; i++)
>>>> + /* XXX: Generate a more secure random string? */
>>>> + packet->vector[i] = random() % 255;
>>>>
>>>> The random seed is initialized at BackendRun() with MyProcPid and
>>>> the time of backend process launched.
>>>> Then, PostgresMain() -> InitPostgres() -> PerformAuthentication()
>>>> will be called, and this random() shall be the first call just after
>>>> initialization of the srandom().
>>>>
>>>> Do you have any good idea?
>>>> Or, do you think it should be fixed with high priority?
>>>
>>> It does need a fairly good random number generator there to be secure,
>>> so it should probably be improved. OTOH, the whole thing can be more
>>> considered obfuscation rather than encryption, and those who really
>>> care about higher security will use ipsec or trusted networks.
>>>
>>> Maybe switching to erand48() would make this better, and good enough?
>>
>> As Tom pointed out, it is fundamentally same.
>> The matter is this random() invocation is the first time after
>> initialization of random seed by srandom(). It means an external observer
>> can estimate the random value uniquely using pid and startup time.
>>
>> In other representation, the "random" value is the result of function
>> which takes arguments of pid and startup time, without random factor.
>>
>> for (i = 0; i< RADIUS_VECTOR_LENGTH; i++)
>> packet->vector[i] = f(getpid(), port->SessionStartTime, i);
>>
>> One idea is to modify the logic to set up random seed in BackendRun().
>> In most of UNIX-like operating system, we can use /dev/random as a random
>> seed which is well randomized.
>>
>> http://en.wikipedia.org/wiki//dev/random
>>
>> It seems to me PostmasterRandom() is a right place to set random seed,
>> and we can inject a block something like #ifdef HAVE_DEV_RANDOM ...
>>
>> Instead of such kind of efforts, we can also document that PostgreSQL and
>> RADIUS server should have communication using enough secure connection
>> explicitly. IMO, it will cover most of use cases.
>
> There is one more option here - use OpenSSL if available. It has
> functions for secure random number generations
> (http://www.openssl.org/docs/crypto/RAND_bytes.html) That seems easy
> enough when OpenSSL is available.

In just my opinion (so, committer may have different one), it is an
option to utilize openSSL library when available. However, it should
be moved to PostmasterRandom() and used to provide more randomness
for srandom(). And, srandom() in the head of BackendRun() should be
replaced by PostmasterRandom().

I also want any opinions from others.

> The question then becomes what do we do if we don't have OpenSSL
> available? Do we document that it's not secure, or refuse to run it?
> I'd vote for document it.. If you don't have SSL enabled, then you
> clearly don't use SSL for the libpq connection, which means the
> password goes in cleartext in that stream...

The seed of random is a different issue from safeness of password on
the stream between client and server. For example, if admin set up
IPsec/ESP between them, OpenSSL is not must-requirement.

Even if OpenSSL is not available, as long as both of PostgreSQL and
RADIUS server are set up in trusted network, we can consider it is
secure. So, all we can do is to introduce the risk, and the decisions
are depending on end-users.

>>>> * It casts char array (such as radius_buffer) into radius_packet
>>>> structure. The radius_packet structure represents the format of
>>>> RADIUS network packet as is.
>>>> It may be preferable to give compiler a hint not to align this
>>>> structure.
>>>> In GCC, we can use "__attribute__((packed))" to suggest not to
>>>> align the member of structure. Is there any portable way for this?
>>>
>>> This I can't answer, I don't know this well enough. Somebody else?
>>
>> What manner is applied to handle network protocol in other part?
>>
>> The radius_packet is declared as follows:
>>
>> + typedef struct
>> + {
>> + unsigned char code; +0
>> + unsigned char id; +1
>> + unsigned short length; +2
>> + unsigned char vector[RADIUS_VECTOR_LENGTH]; +4? +8?
>> + } radius_packet;
>>
>> It may be a bit nervous, except for possible alignment of the vector
>> on 64bit architecture.
>>
>> And, one more. It seems to me uint8 and uint16 are more preferable than
>> unsigned char/short in this context.
>
> Yeha, that is probably right. I copied that off a reference
> implementation of the struct. Will change accordingly.
>
> FWIW, I tested it on Win64 and it didn't have any issues there at least.

Just to be safe, could you inject an Assert() here?
If a minor compiler aligned it unintentionally, it will be a bug not easy
to find out.

/* check whether the compiler aligned it unintentionally, or not */
Assert(offsetof(radius_packet, vector) == 4);

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>