[openssl.org #3345] potential bug in crypto/evp/bio_b64.c

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[openssl.org #3345] potential bug in crypto/evp/bio_b64.c

Rich Salz via RT
Coverity run has uncovered the following use of uninitialized local
variable in b64_read(). This applies to both 1.0.1g and master branch:

Error: UNINIT:
crypto/evp/bio_b64.c:146:
var_decl: Declaring variable "num" without initializer.
path:crypto/evp/bio_b64.c:150:
cond_false: Condition "out == NULL", taking false branch
path:crypto/evp/bio_b64.c:150:
if_end: End of if statement
path:crypto/evp/bio_b64.c:153:
cond_false: Condition "ctx == NULL", taking false branch
path:crypto/evp/bio_b64.c:153:
cond_false: Condition "b->next_bio == NULL", taking false branch
path:crypto/evp/bio_b64.c:153:
if_end: End of if statement
path:crypto/evp/bio_b64.c:157:
cond_true: Condition "ctx->encode != 2", taking true branch
path:crypto/evp/bio_b64.c:167:
cond_false: Condition "ctx->buf_len > 0", taking false branch
path:crypto/evp/bio_b64.c:183:
if_end: End of if statement
path:crypto/evp/bio_b64.c:189:
cond_true: Condition "outl > 0", taking true branch
path:crypto/evp/bio_b64.c:191:
cond_false: Condition "ctx->cont <= 0", taking false branch
path:crypto/evp/bio_b64.c:192:
if_end: End of if statement
path:crypto/evp/bio_b64.c:197:
cond_false: Condition "i <= 0", taking false branch
path:crypto/evp/bio_b64.c:215:
if_end: End of if statement
path:crypto/evp/bio_b64.c:221:
cond_true: Condition "ctx->start", taking true branch
path:crypto/evp/bio_b64.c:221:
cond_true: Condition "BIO_test_flags(b, -1 /* ~0 */) & 0x100", taking true branch
path:crypto/evp/bio_b64.c:225:
if_fallthrough: Falling through to end of if statement
path:crypto/evp/bio_b64.c:303:
cond_true: Condition "BIO_test_flags(b, -1 /* ~0 */) & 0x100", taking true branch
path:crypto/evp/bio_b64.c:314:
cond_true: Condition "jj > 2", taking true branch
path:crypto/evp/bio_b64.c:316:
cond_false: Condition "ctx->tmp[jj - 1] == '='", taking false branch
path:crypto/evp/bio_b64.c:321:
if_end: End of if statement
path:crypto/evp/bio_b64.c:325:
cond_true: Condition "jj != i", taking true branch
path:crypto/evp/bio_b64.c:331:
cond_false: Condition "z > 0", taking false branch
path:crypto/evp/bio_b64.c:334:
if_end: End of if statement
path:crypto/evp/bio_b64.c:336:
if_fallthrough: Falling through to end of if statement
path:crypto/evp/bio_b64.c:343:
if_end: End of if statement
path:crypto/evp/bio_b64.c:345:
cond_false: Condition "i < 0", taking false branch
path:crypto/evp/bio_b64.c:350:
if_end: End of if statement
path:crypto/evp/bio_b64.c:352:
cond_true: Condition "ctx->buf_len <= outl", taking true branch
path:crypto/evp/bio_b64.c:353:
if_fallthrough: Falling through to end of if statement
path:crypto/evp/bio_b64.c:355:
if_end: End of if statement
path:crypto/evp/bio_b64.c:360:
cond_true: Condition "ctx->buf_off == ctx->buf_len", taking true branch
path:crypto/evp/bio_b64.c:367:
loop: Jumping back to the beginning of the loop
path:crypto/evp/bio_b64.c:189:
loop_begin: Jumped back to beginning of loop
path:crypto/evp/bio_b64.c:189:
cond_true: Condition "outl > 0", taking true branch
path:crypto/evp/bio_b64.c:191:
cond_false: Condition "ctx->cont <= 0", taking false branch
path:crypto/evp/bio_b64.c:192:
if_end: End of if statement
path:crypto/evp/bio_b64.c:197:
cond_false: Condition "i <= 0", taking false branch
path:crypto/evp/bio_b64.c:215:
if_end: End of if statement
path:crypto/evp/bio_b64.c:221:
cond_true: Condition "ctx->start", taking true branch
path:crypto/evp/bio_b64.c:221:
cond_true: Condition "BIO_test_flags(b, -1 /* ~0 */) & 0x100", taking true branch
path:crypto/evp/bio_b64.c:225:
if_fallthrough: Falling through to end of if statement
path:crypto/evp/bio_b64.c:303:
cond_true: Condition "BIO_test_flags(b, -1 /* ~0 */) & 0x100", taking true branch
path:crypto/evp/bio_b64.c:314:
cond_false: Condition "jj > 2", taking false branch
path:crypto/evp/bio_b64.c:322:
if_end: End of if statement
path:crypto/evp/bio_b64.c:325:
cond_true: Condition "jj != i", taking true branch
path:crypto/evp/bio_b64.c:331:
cond_false: Condition "z > 0", taking false branch
path:crypto/evp/bio_b64.c:334:
if_end: End of if statement
path:crypto/evp/bio_b64.c:336:
if_fallthrough: Falling through to end of if statement
path:crypto/evp/bio_b64.c:343:
if_end: End of if statement
path:crypto/evp/bio_b64.c:345:
cond_false: Condition "i < 0", taking false branch
path:crypto/evp/bio_b64.c:350:
if_end: End of if statement
path:crypto/evp/bio_b64.c:352:
cond_true: Condition "ctx->buf_len <= outl", taking true branch
path:crypto/evp/bio_b64.c:353:
if_fallthrough: Falling through to end of if statement
path:crypto/evp/bio_b64.c:355:
if_end: End of if statement
path:crypto/evp/bio_b64.c:360:
cond_true: Condition "ctx->buf_off == ctx->buf_len", taking true branch
path:crypto/evp/bio_b64.c:367:
loop: Jumping back to the beginning of the loop
path:crypto/evp/bio_b64.c:189:
loop_begin: Jumped back to beginning of loop
path:crypto/evp/bio_b64.c:189:
cond_true: Condition "outl > 0", taking true branch
path:crypto/evp/bio_b64.c:191:
cond_false: Condition "ctx->cont <= 0", taking false branch
path:crypto/evp/bio_b64.c:192:
if_end: End of if statement
path:crypto/evp/bio_b64.c:197:
cond_false: Condition "i <= 0", taking false branch
path:crypto/evp/bio_b64.c:215:
if_end: End of if statement
path:crypto/evp/bio_b64.c:221:
cond_true: Condition "ctx->start", taking true branch
path:crypto/evp/bio_b64.c:221:
cond_true: Condition "BIO_test_flags(b, -1 /* ~0 */) & 0x100", taking true branch
path:crypto/evp/bio_b64.c:225:
if_fallthrough: Falling through to end of if statement
path:crypto/evp/bio_b64.c:303:
cond_true: Condition "BIO_test_flags(b, -1 /* ~0 */) & 0x100", taking true branch
path:crypto/evp/bio_b64.c:314:
cond_true: Condition "jj > 2", taking true branch
path:crypto/evp/bio_b64.c:316:
cond_true: Condition "ctx->tmp[jj - 1] == '='", taking true branch
path:crypto/evp/bio_b64.c:319:
cond_true: Condition "ctx->tmp[jj - 2] == '='", taking true branch
path:crypto/evp/bio_b64.c:325:
cond_true: Condition "jj != i", taking true branch
path:crypto/evp/bio_b64.c:331:
cond_true: Condition "z > 0", taking true branch
path:crypto/evp/bio_b64.c:336:
if_fallthrough: Falling through to end of if statement
path:crypto/evp/bio_b64.c:343:
if_end: End of if statement
path:crypto/evp/bio_b64.c:345:
cond_false: Condition "i < 0", taking false branch
path:crypto/evp/bio_b64.c:350:
if_end: End of if statement
path:crypto/evp/bio_b64.c:352:
cond_true: Condition "ctx->buf_len <= outl", taking true branch
path:crypto/evp/bio_b64.c:353:
if_fallthrough: Falling through to end of if statement
path:crypto/evp/bio_b64.c:355:
if_end: End of if statement
path:crypto/evp/bio_b64.c:360:
cond_true: Condition "ctx->buf_off == ctx->buf_len", taking true branch
path:crypto/evp/bio_b64.c:367:
loop: Jumping back to the beginning of the loop
path:crypto/evp/bio_b64.c:189:
loop_begin: Jumped back to beginning of loop
path:crypto/evp/bio_b64.c:189:
cond_true: Condition "outl > 0", taking true branch
path:crypto/evp/bio_b64.c:191:
cond_false: Condition "ctx->cont <= 0", taking false branch
path:crypto/evp/bio_b64.c:192:
if_end: End of if statement
path:crypto/evp/bio_b64.c:197:
cond_false: Condition "i <= 0", taking false branch
path:crypto/evp/bio_b64.c:215:
if_end: End of if statement
path:crypto/evp/bio_b64.c:221:
cond_true: Condition "ctx->start", taking true branch
path:crypto/evp/bio_b64.c:221:
cond_true: Condition "BIO_test_flags(b, -1 /* ~0 */) & 0x100", taking true branch
path:crypto/evp/bio_b64.c:225:
if_fallthrough: Falling through to end of if statement
path:crypto/evp/bio_b64.c:303:
cond_true: Condition "BIO_test_flags(b, -1 /* ~0 */) & 0x100", taking true branch
path:crypto/evp/bio_b64.c:314:
cond_true: Condition "jj > 2", taking true branch
path:crypto/evp/bio_b64.c:316:
cond_true: Condition "ctx->tmp[jj - 1] == '='", taking true branch
path:crypto/evp/bio_b64.c:319:
cond_true: Condition "ctx->tmp[jj - 2] == '='", taking true branch
path:crypto/evp/bio_b64.c:325:
cond_false: Condition "jj != i", taking false branch
path:crypto/evp/bio_b64.c:329:
if_end: End of if statement
path:crypto/evp/bio_b64.c:331:
cond_true: Condition "z > 0", taking true branch
path:crypto/evp/bio_b64.c:336:
if_fallthrough: Falling through to end of if statement
path:crypto/evp/bio_b64.c:343:
if_end: End of if statement
path:crypto/evp/bio_b64.c:345:
cond_false: Condition "i < 0", taking false branch
path:crypto/evp/bio_b64.c:350:
if_end: End of if statement
path:crypto/evp/bio_b64.c:352:
cond_true: Condition "ctx->buf_len <= outl", taking true branch
path:crypto/evp/bio_b64.c:353:
if_fallthrough: Falling through to end of if statement
path:crypto/evp/bio_b64.c:355:
if_end: End of if statement
path:crypto/evp/bio_b64.c:360:
cond_false: Condition "ctx->buf_off == ctx->buf_len", taking false branch
path:crypto/evp/bio_b64.c:364:
if_end: End of if statement
path:crypto/evp/bio_b64.c:367:
loop: Jumping back to the beginning of the loop
path:crypto/evp/bio_b64.c:189:
loop_begin: Jumped back to beginning of loop
path:crypto/evp/bio_b64.c:189:
cond_true: Condition "outl > 0", taking true branch
path:crypto/evp/bio_b64.c:191:
cond_false: Condition "ctx->cont <= 0", taking false branch
path:crypto/evp/bio_b64.c:192:
if_end: End of if statement
path:crypto/evp/bio_b64.c:197:
cond_false: Condition "i <= 0", taking false branch
path:crypto/evp/bio_b64.c:215:
if_end: End of if statement
path:crypto/evp/bio_b64.c:221:
cond_true: Condition "ctx->start", taking true branch
path:crypto/evp/bio_b64.c:221:
cond_false: Condition "BIO_test_flags(b, -1 /* ~0 */) & 0x100", taking false branch
path:crypto/evp/bio_b64.c:226:
cond_true: Condition "ctx->start", taking true branch
path:crypto/evp/bio_b64.c:229:
cond_true: Condition "j < i", taking true branch
path:crypto/evp/bio_b64.c:231:
cond_true: Condition "*q++ != 10", taking true branch
path:crypto/evp/bio_b64.c:231:
continue: Continuing loop
path:crypto/evp/bio_b64.c:264:
loop: Looping back
path:crypto/evp/bio_b64.c:229:
cond_true: Condition "j < i", taking true branch
path:crypto/evp/bio_b64.c:231:
cond_true: Condition "*q++ != 10", taking true branch
path:crypto/evp/bio_b64.c:231:
continue: Continuing loop
path:crypto/evp/bio_b64.c:264:
loop: Looping back
path:crypto/evp/bio_b64.c:229:
cond_true: Condition "j < i", taking true branch
path:crypto/evp/bio_b64.c:231:
cond_false: Condition "*q++ != 10", taking false branch
path:crypto/evp/bio_b64.c:231:
if_end: End of if statement
path:crypto/evp/bio_b64.c:237:
cond_true: Condition "ctx->tmp_nl", taking true branch
path:crypto/evp/bio_b64.c:241:
continue: Continuing loop
path:crypto/evp/bio_b64.c:264:
loop: Looping back
path:crypto/evp/bio_b64.c:229:
cond_false: Condition "j < i", taking false branch
path:crypto/evp/bio_b64.c:264:
loop_end: Reached end of loop
path:crypto/evp/bio_b64.c:267:
cond_true: Condition "j == i", taking true branch
crypto/evp/bio_b64.c:267:
uninit_use: Using uninitialized value "num".

Total of 1 unclassified defects


Here is another, hopefully more readable representation of the report:

144static int b64_read(BIO *b, char *out, int outl)
145        {
   
1. var_decl: Declaring variable "num" without initializer.
146        int ret=0,i,ii,j,k,x,n,num,ret_code=0;
147        BIO_B64_CTX *ctx;
148        unsigned char *p,*q;
149
   
2. Condition "out == NULL", taking false branch
150        if (out == NULL) return(0);
151        ctx=(BIO_B64_CTX *)b->ptr;
152
   
3. Condition "ctx == NULL", taking false branch
   
4. Condition "b->next_bio == NULL", taking false branch
153        if ((ctx == NULL) || (b->next_bio == NULL)) return(0);
154
155        BIO_clear_retry_flags(b);
156
   
5. Condition "ctx->encode != 2", taking true branch
157        if (ctx->encode != B64_DECODE)
158                {
159                ctx->encode=B64_DECODE;
160                ctx->buf_len=0;
161                ctx->buf_off=0;
162                ctx->tmp_len=0;
163                EVP_DecodeInit(&(ctx->base64));
164                }
165
166        /* First check if there are bytes decoded/encoded */
   
6. Condition "ctx->buf_len > 0", taking false branch
167        if (ctx->buf_len > 0)
168                {
169                OPENSSL_assert(ctx->buf_len >= ctx->buf_off);
170                i=ctx->buf_len-ctx->buf_off;
171                if (i > outl) i=outl;
172                OPENSSL_assert(ctx->buf_off+i < (int)sizeof(ctx->buf));
173                memcpy(out,&(ctx->buf[ctx->buf_off]),i);
174                ret=i;
175                out+=i;
176                outl-=i;
177                ctx->buf_off+=i;
178                if (ctx->buf_len == ctx->buf_off)
179                        {
180                        ctx->buf_len=0;
181                        ctx->buf_off=0;
182                        }
183                }
184
185        /* At this point, we have room of outl bytes and an empty
186         * buffer, so we should read in some more. */
187
188        ret_code=0;
   
7. Condition "outl > 0", taking true branch
   
24. Condition "outl > 0", taking true branch
   
40. Condition "outl > 0", taking true branch
   
58. Condition "outl > 0", taking true branch
   
76. Condition "outl > 0", taking true branch
189        while (outl > 0)
190                {
   
8. Condition "ctx->cont <= 0", taking false branch
   
25. Condition "ctx->cont <= 0", taking false branch
   
41. Condition "ctx->cont <= 0", taking false branch
   
59. Condition "ctx->cont <= 0", taking false branch
   
77. Condition "ctx->cont <= 0", taking false branch
191                if (ctx->cont <= 0)
192                        break;
193
194                i=BIO_read(b->next_bio,&(ctx->tmp[ctx->tmp_len]),
195                        B64_BLOCK_SIZE-ctx->tmp_len);
196
   
9. Condition "i <= 0", taking false branch
   
26. Condition "i <= 0", taking false branch
   
42. Condition "i <= 0", taking false branch
   
60. Condition "i <= 0", taking false branch
   
78. Condition "i <= 0", taking false branch
197                if (i <= 0)
198                        {
199                        ret_code=i;
200
201                        /* Should we continue next time we are called? */
202                        if (!BIO_should_retry(b->next_bio))
203                                {
204                                ctx->cont=i;
205                                /* If buffer empty break */
206                                if(ctx->tmp_len == 0)
207                                        break;
208                                /* Fall through and process what we have */
209                                else
210                                        i = 0;
211                                }
212                        /* else we retry and add more data to buffer */
213                        else
214                                break;
215                        }
216                i+=ctx->tmp_len;
217                ctx->tmp_len = i;
218
219                /* We need to scan, a line at a time until we
220                 * have a valid line if we are starting. */
   
10. Condition "ctx->start", taking true branch
   
11. Condition "BIO_test_flags(b, -1 /* ~0 */) & 0x100", taking true branch
   
27. Condition "ctx->start", taking true branch
   
28. Condition "BIO_test_flags(b, -1 /* ~0 */) & 0x100", taking true branch
   
43. Condition "ctx->start", taking true branch
   
44. Condition "BIO_test_flags(b, -1 /* ~0 */) & 0x100", taking true branch
   
61. Condition "ctx->start", taking true branch
   
62. Condition "BIO_test_flags(b, -1 /* ~0 */) & 0x100", taking true branch
   
79. Condition "ctx->start", taking true branch
   
80. Condition "BIO_test_flags(b, -1 /* ~0 */) & 0x100", taking false branch
221                if (ctx->start && (BIO_get_flags(b) & BIO_FLAGS_BASE64_NO_NL))
222                        {
223                        /* ctx->start=1; */
224                        ctx->tmp_len=0;
   
12. Falling through to end of if statement
   
29. Falling through to end of if statement
   
45. Falling through to end of if statement
   
63. Falling through to end of if statement
225                        }
   
81. Condition "ctx->start", taking true branch
226                else if (ctx->start)
227                        {
228                        q=p=(unsigned char *)ctx->tmp;
   
82. Condition "j < i", taking true branch
   
85. Condition "j < i", taking true branch
   
88. Condition "j < i", taking true branch
   
92. Condition "j < i", taking false branch
229                        for (j=0; j<i; j++)
230                                {
   
83. Condition "*q++ != 10", taking true branch
   
84. Continuing loop
   
86. Condition "*q++ != 10", taking true branch
   
87. Continuing loop
   
89. Condition "*q++ != 10", taking false branch
231                                if (*(q++) != '\n') continue;
232
233                                /* due to a previous very long line,
234                                 * we need to keep on scanning for a '\n'
235                                 * before we even start looking for
236                                 * base64 encoded stuff. */
   
90. Condition "ctx->tmp_nl", taking true branch
237                                if (ctx->tmp_nl)
238                                        {
239                                        p=q;
240                                        ctx->tmp_nl=0;
   
91. Continuing loop
241                                        continue;
242                                        }
243
244                                k=EVP_DecodeUpdate(&(ctx->base64),
245                                        (unsigned char *)ctx->buf,
246                                        &num,p,q-p);
247                                if ((k <= 0) && (num == 0) && (ctx->start))
248                                        EVP_DecodeInit(&ctx->base64);
249                                else
250                                        {
251                                        if (p != (unsigned char *)
252                                                &(ctx->tmp[0]))
253                                                {
254                                                i-=(p- (unsigned char *)
255                                                        &(ctx->tmp[0]));
256                                                for (x=0; x < i; x++)
257                                                        ctx->tmp[x]=p[x];
258                                                }
259                                        EVP_DecodeInit(&ctx->base64);
260                                        ctx->start=0;
261                                        break;
262                                        }
263                                p=q;
264                                }
265
266                        /* we fell off the end without starting */
   
93. Condition "j == i", taking true branch
   
CID 102891 (#1 of 5): Uninitialized scalar variable (UNINIT)94. uninit_use: Using uninitialized value "num".
267                        if ((j == i) && (num == 0))


Hope this helps.

______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #3345] potential bug in crypto/evp/bio_b64.c

Tim Hudson
On 6/05/2014 1:13 PM, Arthur Mesh via RT wrote:
> Coverity run has uncovered the following use of uninitialized local
> variable in b64_read(). This applies to both 1.0.1g and master branch:

Arthur - what version of the coverity analysis tools are you running?
I don't see this in the current scan.coverity.com reported list of items
so either it has been previously looked at or your configuration is
different or your version of tools is different or you are running
across a more recent source drop than we have put into scan.coverity.com
(I suspect the latter is the issue).

If you refer to this issue you will see where the code was introduced.

https://rt.openssl.org/Ticket/Display.html?id=3289

diff --git a/crypto/evp/bio_b64.c b/crypto/evp/bio_b64.c
index 72a2a67..ac6d441 100644
--- a/crypto/evp/bio_b64.c
+++ b/crypto/evp/bio_b64.c
@@ -264,7 +264,7 @@ static int b64_read(BIO *b, char *out, int outl)
                                }
 
                        /* we fell off the end without starting */
-                       if (j == i)
+                       if ((j == i) && (num == 0))


There needs to be a corresponding num=0 initialisation prior to the
immediately preceding for loop.

I have re-opened that RT issue and cross-referenced both the new RT
issue and the original one which introduced the patch.

Thanks,
Tim.


______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #3345] potential bug in crypto/evp/bio_b64.c

Arthur Mesh
On Tue, May 06, 2014 at 04:09:10PM +1000, Tim Hudson wrote:
> Arthur - what version of the coverity analysis tools are you running?

Tim,

I am using:
Coverity Build Capture version 6.6.1 on FreeBSD 8.4-STABLE i386

I see there is a 3ba1e406c2309adb427ced9815ebf05f5b58d155 that
supposedly fixes this defect.

Thanks
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]