Skip to content
This repository has been archived by the owner on May 14, 2024. It is now read-only.

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

Open
axkibe opened this issue Nov 23, 2023 · 18 comments
Labels

Comments

@axkibe
Copy link

axkibe commented Nov 23, 2023

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
Copy link
Member

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
Copy link
Author

axkibe commented Nov 23, 2023

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
Copy link
Author

axkibe commented Nov 23, 2023

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
Copy link
Member

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
Copy link
Member

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
Copy link
Author

axkibe commented Nov 23, 2023

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

@axkibe
Copy link
Author

axkibe commented Nov 23, 2023

  • removed lower MWE much better -

@jsumners
Copy link
Member

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

@axkibe
Copy link
Author

axkibe commented Nov 23, 2023 via email

@axkibe
Copy link
Author

axkibe commented Nov 23, 2023

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
Copy link
Member

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
Copy link
Author

axkibe commented Nov 24, 2023

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
Copy link
Member

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.

@jsumners jsumners added the bug label Nov 24, 2023
@axkibe
Copy link
Author

axkibe commented Nov 24, 2023

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
Copy link
Member

jsumners commented Nov 24, 2023

node-ldapjs/LICENSE

Lines 13 to 19 in 6ceef13

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
THE SOFTWARE

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
Copy link
Author

axkibe commented Nov 24, 2023

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.

@axkibe axkibe closed this as completed Nov 24, 2023
@jsumners jsumners reopened this Nov 24, 2023
@jsumners
Copy link
Member

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
Copy link
Author

axkibe commented Nov 25, 2023

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants