View Revisions: Issue #595

Summary 0000595: Incorrect message length validation in do_SCRAM() - backend authentication
Revision 2020-03-13 08:52 by t-ishii
Description There is an incorrect message length validation performed during backend authentication in the do_SCRAM() function (src/auth/pool_auth.c).

               /* Read next packend */
                pool_read(backend, &kind, sizeof(kind));
                pool_read(backend, &len, sizeof(len));
                if (kind != 'R')
                        ereport(ERROR,
                                        (errmsg("backend authentication failed"),
                                         errdetail("backend response with kind \'%c\' when expecting \'R\'", kind)));
                message_length = ntohl(len);
                if (len <= 8)
                        ereport(ERROR,
                                        (errmsg("backend authentication failed"),
                                         errdetail("backend response with no data ")));


The code is currently checking if "len <= 8", but len is is network-byte-order (big-endian).
It is surely meant to be checking "message_length" instead, which is "len" coverted to host-byte-order (see previous line of code).
Under (Intel) Linux, which is little-endian, the value of "len" will be a large number and thus render the current error condition check ineffective [for example, in one case that I debugged, an example value of len was 134217728 (0x08000000), meaning that message_length was actually 8].
Additionally, it seems the "<=" check should actually be "<", based on the length values that I see when debugging this code.
I've attached a proposed patch (pgpool2_pool_auth_do_scram.patch).
Please review.
Revision 2020-03-12 12:18 by gregn123
Description There is an incorrect message length validation performed during backend authentication in the do_SCRAM() function (src/auth/pool_auth.c).

               /* Read next packend */
                pool_read(backend, &kind, sizeof(kind));
                pool_read(backend, &len, sizeof(len));
                if (kind != 'R')
                        ereport(ERROR,
                                        (errmsg("backend authentication failed"),
                                         errdetail("backend response with kind \'%c\' when expecting \'R\'", kind)));
                message_length = ntohl(len);
                if (len <= 8)
                        ereport(ERROR,
                                        (errmsg("backend authentication failed"),
                                         errdetail("backend response with no data ")));


The code is currently checking if "len <= 8", but len is is network-byte-order (big-endian).
It is surely meant to be checking "message_length" instead, which is "len" coverted to host-byte-order (see previous line of code).
Under (Intel) Linux, which is little-endian, the value of "len" will be a large number and thus render the current error condition check ineffective [for example, in one case that I debugged, an example value of len was 134217728 (0x08000000), meaning that message_length was actually 8].
Additionally, it seems the "<=" check should actually be "<", based on the length values that I see when debugging this code.
I've attached a proposed patch (pgpool2_pool_auth_do_scram.patch).
Please review.