Commit b247286
committed
bugfix: check sslhandshake result on immediate OK
This patch fixes the immediate `FFI_OK` path in `tcpsock:sslhandshake()`.
Currently `sslhandshake()` treats the initial `FFI_OK` return from
`ngx_http_lua_ffi_socket_tcp_sslhandshake()` as the final successful result when
`reused_session == false`:
```lua
rc = ngx_http_lua_ffi_socket_tcp_sslhandshake(...)
if rc == FFI_OK then
if reused_session == false then
return true
end
rc = ngx_http_lua_ffi_socket_tcp_get_sslhandshake_result(...)
end
```
This skips `ngx_http_lua_ffi_socket_tcp_get_sslhandshake_result()` for the
`reused_session == false` case.
That is unsafe because the first FFI call result and the final SSL handshake
result are not the same thing.
The first return value tells Lua how the handshake operation progressed:
```text
FFI_ERROR -> immediate call failed
FFI_DONE -> reused session
FFI_OK -> handshake operation completed immediately
FFI_AGAIN -> handshake needs to yield and resume later
```
The final result must still be read from:
```text
ngx_http_lua_ffi_socket_tcp_get_sslhandshake_result(...)
```
This matters when the C side completes the handshake path immediately but records
an SSL error in the cosocket state. For example, certificate verification can fail
inside the handshake handler and set:
```text
u->error_ret
u->openssl_error_code_ret
```
The old Lua code can miss that state.
Old flow:
```text
Lua sslhandshake()
|
v
C: ngx_http_lua_ffi_socket_tcp_sslhandshake(...)
|
v
returns FFI_OK
|
v
reused_session == false?
|
v
return true
|
v
final handshake result is never checked
```
So callers may believe TLS succeeded even though the C cosocket state already has
a handshake error. The next socket operation can then fail with a misleading error,
for example a write failure, instead of returning the original SSL handshake error.
The fixed flow always reads the final result for both immediate and async
completion paths:
```text
Lua sslhandshake()
|
v
C: ngx_http_lua_ffi_socket_tcp_sslhandshake(...)
|
+-----------------------------+
| |
v v
FFI_OK FFI_AGAIN
immediate async handshake
completion |
| v
| co_yield()
| |
+------------+-------------+
|
v
get_sslhandshake_result()
|
v
final result
|
+--------+--------+
| |
v v
FFI_ERROR FFI_OK
| |
v v
return SSL error return true/session
```
This patch separates the two meanings by using:
```text
rc -> result of ngx_http_lua_ffi_socket_tcp_sslhandshake()
res -> result of ngx_http_lua_ffi_socket_tcp_get_sslhandshake_result()
```
The new control flow is:
```lua
rc = ngx_http_lua_ffi_socket_tcp_sslhandshake(...)
if rc == FFI_ERROR then
return nil, err
end
if rc == FFI_DONE then
return reused_session
end
local res
if rc == FFI_OK then
res = ngx_http_lua_ffi_socket_tcp_get_sslhandshake_result(...)
else
assert(rc == FFI_AGAIN)
co_yield()
res = ngx_http_lua_ffi_socket_tcp_get_sslhandshake_result(...)
end
if res == FFI_ERROR then
return nil, ssl_err
end
assert(res == FFI_OK)
return true or session
```
This keeps the existing async behavior, but makes the immediate `FFI_OK` path
follow the same final-result check as the resumed `FFI_AGAIN` path.1 parent e68e857 commit b247286
1 file changed
Lines changed: 30 additions & 32 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
424 | 424 | | |
425 | 425 | | |
426 | 426 | | |
427 | | - | |
428 | | - | |
429 | | - | |
| 427 | + | |
| 428 | + | |
| 429 | + | |
430 | 430 | | |
431 | 431 | | |
432 | | - | |
433 | | - | |
| 432 | + | |
434 | 433 | | |
435 | 434 | | |
436 | | - | |
437 | | - | |
438 | | - | |
439 | | - | |
440 | | - | |
441 | | - | |
442 | | - | |
443 | | - | |
444 | | - | |
445 | | - | |
446 | | - | |
447 | | - | |
| 435 | + | |
| 436 | + | |
| 437 | + | |
448 | 438 | | |
449 | | - | |
450 | | - | |
451 | | - | |
452 | | - | |
| 439 | + | |
453 | 440 | | |
454 | | - | |
455 | | - | |
| 441 | + | |
| 442 | + | |
| 443 | + | |
| 444 | + | |
| 445 | + | |
456 | 446 | | |
457 | | - | |
| 447 | + | |
458 | 448 | | |
459 | | - | |
460 | | - | |
461 | | - | |
| 449 | + | |
| 450 | + | |
| 451 | + | |
462 | 452 | | |
463 | | - | |
| 453 | + | |
| 454 | + | |
| 455 | + | |
464 | 456 | | |
| 457 | + | |
| 458 | + | |
465 | 459 | | |
466 | | - | |
| 460 | + | |
467 | 461 | | |
468 | | - | |
| 462 | + | |
| 463 | + | |
| 464 | + | |
469 | 465 | | |
470 | | - | |
471 | | - | |
| 466 | + | |
| 467 | + | |
472 | 468 | | |
| 469 | + | |
| 470 | + | |
473 | 471 | | |
474 | 472 | | |
475 | 473 | | |
| |||
0 commit comments