ldapjs / node-ldapjs

LDAP Client and Server API for node.js
http://ldapjs.org
MIT License
1.61k stars 448 forks source link

VError: Parser error for undefined:undefined: options.value must be a Buffer or Object #956

Open axkibe opened 11 months ago

axkibe commented 11 months ago

Hello, my ldapjs implemented LDAP server crashed with following backtrace


node:events:493
      throw er; // Unhandled 'error' event
      ^
VError: Parser error for undefined:undefined: options.value must be a Buffer or Object
    at Parser.<anonymous> (/home/data/datanode/node_modules/ldapjs/lib/server.js:459:26)
    at Parser.emit (node:events:515:28)
    at Parser.write (/home/data/datanode/node_modules/ldapjs/lib/messages/parser.js:131:10)
    at Socket.<anonymous> (/home/data/datanode/node_modules/ldapjs/lib/server.js:474:19)
    at Socket.emit (node:events:515:28)
    at addChunk (node:internal/streams/readable:545:12)
    at readableAddChunkPushByteMode (node:internal/streams/readable:495:3)
    at Readable.push (node:internal/streams/readable:375:5)
    at Pipe.onStreamRead (node:internal/stream_base_commons:190:23)
Emitted 'error' event on Server instance at:
    at Parser.<anonymous> (/home/data/datanode/node_modules/ldapjs/lib/server.js:459:12)
    at Parser.emit (node:events:515:28)
    [... lines matching original stack trace ...]
    at Pipe.onStreamRead (node:internal/stream_base_commons:190:23) {
  jse_shortmsg: 'Parser error for undefined:undefined',
  jse_cause: TypeError: options.value must be a Buffer or Object
      at new PasswordPolicyControl (/home/data/datanode/node_modules/@ldapjs/controls/lib/controls/password-policy-control.js:55:13)
      at getControl (/home/data/datanode/node_modules/@ldapjs/controls/index.js:55:19)
      at parseToMessage (/home/data/datanode/node_modules/@ldapjs/messages/lib/parse-to-message.js:71:17)
      at LdapMessage.parse (/home/data/datanode/node_modules/@ldapjs/messages/lib/ldap-message.js:262:41)
      at Parser.write (/home/data/datanode/node_modules/ldapjs/lib/messages/parser.js:117:38)
      at Socket.<anonymous> (/home/data/datanode/node_modules/ldapjs/lib/server.js:474:19)
      at Socket.emit (node:events:515:28)
      at addChunk (node:internal/streams/readable:545:12)
      at readableAddChunkPushByteMode (node:internal/streams/readable:495:3)
      at Readable.push (node:internal/streams/readable:375:5),
  jse_info: {},
  cause: [Function: ve_cause]
}

Node.js v21.1.0

As I just got notified by a recently installed crash&restart notifier I cannot yet reproduce this at will.

At least what I can tell from the log it does not relate to any of my code yet and is triggered purely by some Ldap client on the ldap socket before any of mine provided LDAP server handles gets notified.

Any ideas?

PS: "ldapjs": "^3.0.6",

jsumners commented 11 months ago

Please provide a minimal reproducible example (MRE). Doing so will help us diagnose your issue. It should be the bare minimum code needed to trigger the issue, and easily runnable without any changes or extra code. Please review the integration tests, e.g. issue-940.test.js, for examples of good MREs.

You may use a GitHub repository to host the code if it is too much to fit in a code block (or two).

axkibe commented 11 months ago

As far I can tell it is just nslcd doing its core job of authenticating a user against the LDAP socket. I downgraded node back to v18.14.0 (what I used before) didnt change a thing.

But works with ldapjs 3.0.2 .. broken with 3.05... I'm going to test 3.0.3 and 3.0.4 hoping it helps to narrow down the issue.

axkibe commented 11 months ago

works with ldaps 3.0.2, breaks with ldapjs 3.0.3

I dont know exactly how to make a MRE that involves a bit more complicated setup with nslcd, but IMHO it really happens as soon nslcd tries to bind

jsumners commented 11 months ago

An MRE would include:

  1. A server instance that is just enough to cover the issue
  2. A client instance that sends an equivalent message that triggers the issue
jsumners commented 11 months ago

I recommend using Wireshark to inspect the conversation between your client and server to determine what message is being sent that causes the crash.

axkibe commented 11 months ago

I'm working on a server instance and the mathcing nslcd config file.

axkibe commented 11 months ago
jsumners commented 11 months ago

Please review my original response to this thread. Needing to setup extra services and user accounts is not viable.

axkibe commented 11 months ago

Hmm nslcd really should be a wire compatible target IMO it is the LDAP client on Unix. Anyway if you really need it I can record the crosstalk on the socket and replay it in a mockup client. Will come.

On Thu, Nov 23, 2023, 17:42 James Sumners @.***> wrote:

Please review my original response to this thread. Needing to setup extra services and user accounts is not viable.

— Reply to this email directly, view it on GitHub https://github.com/ldapjs/node-ldapjs/issues/956#issuecomment-1824711935, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADY6LYIPVUIFN7VDQ34N3LYF54HXAVCNFSM6AAAAAA7XQMC7GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRUG4YTCOJTGU . You are receiving this because you authored the thread.Message ID: @.***>

axkibe commented 11 months ago

There you go. A selfcontained and much simpler MWE, as I found out I dont need any of LDAP implementation (at least once I sniffed out the nslcd bind operation, to create it I had to answer to it search operation fitst)

This is all needed:

import fs      from 'node:fs/promises';
import net     from 'node:net';
import util    from 'node:util';
import ldap    from 'ldapjs';

const server = ldap.createServer( );
const listen = util.promisify( server.listen.bind( server ) );

await fs.rm( '/tmp/mwe-socket', { force: true } );
await listen( '/tmp/mwe-socket' );

const client = net.createConnection( { path: '/tmp/mwe-socket' } );

client.write( Buffer.from( '3046020101602202010304157569643d626f622c6f753d6d77652c64633d636f6d8006666f6f626172a01d301b0419312e332e362e312e342e312e34322e322e32372e382e352e31', 'hex' ) );

client.write( Buffer.from( '3081b80201036381b2040d6f753d6d77652c64633d636f6d0a01020a0100020100020100010100a02aa31c040b6f626a656374436c617373040d736861646f774163636f756e74a30a04037569640403626f623066040a736861646f77466c61670409736861646f774d61780409736861646f774d696e0410736861646f774c6173744368616e67650403756964040c736861646f77457870697265040e736861646f77496e616374697665040d736861646f775761726e696e673006020104500103', 'hex' ) );

expected behavior of this code.. hang (as the server will keep listening on the socket and the client socket stays open) behaviour with ldapjs 3.0.6 .. crash (identical to above backtrace)

jsumners commented 11 months ago

It's quite difficult to translate these hex strings back to human readable LDAP messages, and it's just more effort than I am willing to put in. Can you please provide a trace file of the full LDAP conversation those two .write payloads are taken from? This trace file should be able to be opened in Wireshark.

Just looking at the hex dumps in a hex editor, it looks like something has:

  1. Sent a request that has resulted in a response that includes the Password Policy response control included in the response.
  2. A subsequent request is sent that looks for entities matching some filter and wants back a list of specific attributes.
axkibe commented 11 months ago

I didnt use wireshark before, I hacked together a middleman-listener.

Anyway I also changed the code to run over TCP and appended a wirshark dump. mwecreash.zip

FYI: The middleman-listener looked like this:

import fs   from 'node:fs/promises';
import net  from 'node:net';
import util from 'node:util';

function handler( client )
{
    console.log( 'GOT SOCKET' );
    const server = net.createConnection( { path: 'socket' } );

    client.on( 'data', ( data ) => {
        console.log( 'C', data.toString('hex') );
        server.write( data );
    } );

    client.on( 'end', ( ) => {
        server.end( );
    } );

    server.on( 'data', ( data ) => {
        console.log( 'S', data.toString('hex') );
        client.write( data );
    } );

    server.on( 'end', ( ) => {
        client.end( );
    } );
};

const server = net.createServer( handler ) ;
const listen = util.promisify( server.listen.bind( server ) );
process.umask( 0 );
await fs.rm( '/var/run/slapd/ldapi', { force: true } );
await listen( '/var/run/slapd/ldapi' );
process.umask( 0x022 );
console.log( 'cross listening' );

And the complete crosstalk looks like this.

This dump allowed me to use the Buffer writes in the MWE above to create the playback that can realibly crash ldapjs. I mean this should really be the ideal scenario for debugging.

node src/cross.mjs 
cross listening
GOT SOCKET
C 300c020101600702010304008000
S 300c02010161070a010004000400
C 3060020102635b040d6f753d6d77652c64633d636f6d0a01020a0100020100020100010100a029a31b040b6f626a656374436c617373040c706f7369784163636f756e74a30a04037569640403626f623010040375696404097569644e756d626572
S 3081fc0201026481f604157569643d626f622c6f753d6d77652c64633d636f6d3081dc300b0402636e31050403626f62301304096769644e756d6265723106040431303030300c040375696431050403626f62301c040d686f6d654469726563746f7279310b04092f686f6d652f626f623022040b6f626a656374636c6173733113040c706f7369784163636f756e740403746f703022040b6f626a656374436c6173733113040c706f7369784163636f756e740403746f70301304097569644e756d62657231060404313031303014040b646973706c61794e616d6531050403626f623019040a6c6f67696e5368656c6c310b04092f62696e2f62617368
S 300c02010265070a010004000400
GOT SOCKET
C 3043020101601f02010304157569643d626f622c6f753d6d77652c64633d636f6d8003666f6fa01d301b0419312e332e362e312e342e312e34322e322e32372e382e352e31
C 3081b80201036381b2040d6f753d6d77652c64633d636f6d0a01020a0100020100020100010100a02aa31c040b6f626a656374436c617373040d736861646f774163636f756e74a30a04037569640403626f623066040a736861646f77466c61670409736861646f774d61780409736861646f774d696e0410736861646f774c6173744368616e67650403756964040c736861646f77457870697265040e736861646f77496e616374697665040d736861646f775761726e696e673006020104500103
GOT SOCKET

As you see on the second connection the server doesnt get even to answer, thats why I considered it all that is necessary. The first connection is the search query and the ldapjs reply (delivering the requested entry).

Also interestingly while working on this I discovered on ubuntu (mantic) things work as expected, the crash happens only on debian (bookworm).

I hope this is now really all you could ask for.

jsumners commented 11 months ago

This is the minimal reproduction:

'use strict'

const ldapjs = require('ldapjs')

let client
const server = ldapjs.createServer()

server.bind('ou=example', (req, res, next) => {
  res.end()
  return next()
})

server.listen(1666, '127.0.0.1', serverListenCB())

function serverListenCB() {
  client = ldapjs.createClient({ url: 'ldap://127.0.0.1:1666/' })

  const ppControl = new ldapjs.PasswordPolicyControl()
  client.bind('cn=foo,ou=example', 'secret', ppControl, (err) => {
    client.unbind(server.close.bind(server))
  })

}

A pull request to address it is welcome.

axkibe commented 11 months ago

So after all this you push it back to me? Sorry this is getting ridicolous, I'm going to work on dropping this library.. this is practically unmainted if I have to do it all in the end myself, and you just waisted my time. seesh.

jsumners commented 11 months ago

https://github.com/ldapjs/node-ldapjs/blob/6ceef130145b38ba33b5ccc798bf49304194fb0f/LICENSE#L13-L19

No one is required to give you free support. I worked quite diligently to guide you toward finding the cause, and will provide guidance in solving it. Those who are affected by some issue, and need it resolved, will see a resolution much quicker by their own involvement and effort. Otherwise, it will get resolved whenever someone with interest and time decides to take it on.

You're welcome to use whatever software you like. Threats will not induce any different outcome here.

axkibe commented 11 months ago

You were not helping me in guiding in anyway.. if I need to fix it myself say right from the start instead of demanding a MRE. And I invested time to make you a MRE. But you didnt like it. So I made you another one, one you literally just would have needed to run to see where the parser of your lib messed up. But you don't like that MRE, you wanted it exactly the way you like it. So I made you another one.. and then finally you say OK it's a bug I have to fix it. Id that is the outcome say so from the start providing you with MREs to your liking is not "guidance". If I need to fix it myself say so from the start instead of keeping me busy for 2 days for your MREs. I could reproduce it from start. I obviously cannot work with you.

BTW it is a bug most likely you introduced. Also do to your strange decision to have sublibraries with ^ syntax past ldapjs version are not easily reproduceable. As after 3.0.3 it always installed the latest sublibs. And it is a bug with the most important LDAP client there is, so normally and other OpenSource software would be concerned. Heck I even contributed to this lib in the past. Sorry your way handling people is unacceptable. Seeing joyen the homepage I thought this would be a serious library, but it's handled not.

jsumners commented 11 months ago

Whether or not you agree:

  1. We need a reproduction that can be added as a failing test to the test suite in order to ensure that any fix is correct and guard against future regressions.
  2. I provided clear instructions and an example of what such a reproduction would look like.
  3. Those that found an issue are the best positioned to create such a reproduction because they are the ones with the necessary information required to do so.
  4. Once enough information was provided, I was able to create the reproduction as originally requested.

BTW it is a bug most likely you introduced.

It does not matter who introduced any issue.

And it is a bug with the most important LDAP client there is, so normally and other OpenSource software would be concerned.

If no one was concerned, the issue would have been closed as "won't fix."

Seeing joyen the homepage I thought this would be a serious library, but it's handled not.

We do not have any affiliation with Joyent. This project is maintained by a community of volunteers. And without them, this library would not be usable by any current version of Node.js.

You are still welcome to contribute a fix for this issue. Otherwise, it will remain open until someone with time and motivation takes it on.

axkibe commented 11 months ago

We do not have any affiliation with Joyent.

Then you absolutely have to remove the logo from the ldapjs homepage.

And you still fail to realize the issue is pestering me mutliple times about MRE.. without telling me in front, if I have to fix it myself.. this is absolutely inacceptable behaviour.